Closed Bug 464018 Opened 11 years ago Closed 11 years ago

unify url params for pushlog, json-pushes


(Developer Services :: Mercurial:, defect)

Not set


(Not tracked)



(Reporter: ted, Assigned: ted)




(1 file, 1 obsolete file)

Currently the pushlog takes date parameters, as well as fromchangeset,tochangeset, while json-pushes takes only startID,endID, which requires you to know the push IDs, which are unknowable from the outside. We should unify the input for these, allowing us to make the same queries from both and receive the output as HTML/ATOM/JSON.
Yes. I'd love to see us doing something that would allow basically all kinds of params to be grouped together with AND.

You should be able to just enter startID and enddate, for example. I think that should be fairly easy to do.
Assignee: nobody → ted.mielczarek
Ok, this seems to work. I need to write some unit tests to verify this doesn't break things though.
I'm wondering, would it be bad to allow multiple start and end params? It's somewhat of an edgecase, I just wonder if there's a good reason to exclude it. I'd just all " AND " them together. One could even do something sensible for startID=3&startID=5, where sensible is really just requiring both (as for dates, it'd be much harder to deduce what the AND means in the code). And as it's likely going to stay an edgecase, I'd avoid premature optimizations and just pass all to sqlite.
What would be the point, aside from adding code complexity?
Not sure if it added complexity, really. You'd not have the elif part in the param handling, which, if given multiple options, has an interesting idea of prioritizing the different options. So I'd expect the code to give a more stable feedback against funny combinations of params.

I'm not sure if there is an actual real life use case for specifying both a start date and a start push, for example. I just found the elif hard to read and possibly surprising if someone ends up coding something that doesn't resolve those funkiness upfront.
Pike, can you give this a once-over? I didn't implement your multiple-parameter-handling, I'm just unsure of the value there. I added a unittest framework:

And then fixed all the bugs I had introduced. I also fixed bug 449377 while I was in there, and added a test for it.
Attachment #348806 - Attachment is obsolete: true
Attachment #349315 - Flags: review?(l10n)
Comment on attachment 349315 [details] [diff] [review]
unify query parameter handling among all output methods, pass tests


Pairing this with bug 466149 would be good, in particular if I'd use this to just hit all l10n repos instead of just a few selected ones.

The html output has some left issues with date ranges, as per irc.

I'd make the non-optional arguments in PushlogQuery.__init__ non-optional, and I think that at least the page and dates args shouldn't be args.

I'd default queryend and queryend_value to None, as you're not using them in the default case anyway.

For the unit tests, the good news is that they run on a Mac in Berlin, too. Is the timezone stuff for NY needed? I would have expected that to be happening before launching the web interface, too. My perf-me would love to factor the repo setup and hg serve into something that isn't set up and torn down for each individual test, but that ain't critical, nor do I have a good alternative to suggest.
Attachment #349315 - Flags: review?(l10n) → review+
I don't know, to be honest. I would have expected the TZ stuff to matter, but it looks like I've put it in the wrong place. Do the tests pass if you remove that line?
to paste from #developers, the TZ stuff does matter, as the pushlog query parsing is in server localtime, and the unit test queries don't specify a timezone. Nor is it clear that they could.

Wether it matters when that env is set is unclear to me, it works here either before or after starting hg serve.
And addressed your review comments in a followup push, because I forgot to qrefresh:
Plus a small hg templates change to go along with:
Closed: 11 years ago
Resolution: --- → FIXED
Product: → Release Engineering
Product: Release Engineering → Developer Services
You need to log in before you can comment on or make changes to this bug.