Minor parseint refactoring, tweaks

[dannon]

Followup to #676 commentary, ping @nsoranzo @carlfeberhard

[nsoranzo]

👍
Thanks @dannon!

[carlfeberhard]

• You failed to change the keyword 'min' in the view_multiple method and the page this was meant to support now errors
• Even if that is fixed, how do I call 'http://localhost:8080/history/view_multiple?limit=None' and load all the histories at once now?
• What if I had actually wanted to raise the value error if a user called 'http://localhost:8080/history/view_multiple?limit=blah' (thus the kwarg.get( 'default' ))

This was one minor line in a 700 line pull request where no mention was made of the major enhancement it was part of, design decisions, architecture, performance, border-cases, use-cases, or even the overarching issue of prioritizing this over other issues. No - at the minimum 2-3 man hours were dedicated to the use of a ternary operator that parses a string.

I'm not sure why you two believe I might not think about these things or have reasons for them. I'm not sure why these changes were considered necessary or wise.

Nicola and Dannon - respectfully - please try to decrease the amount of stylistic changes/requests on my pull requests. I do not find them useful and their improvements to Galaxy are debatable. I'm fine if you want me to fix bugs, add docs/comments, etc. But, as far as I'm aware, the pull request system is meant to find bugs before they're committed and keep everyone aware of changes (since we don't use roadmaps and have no means of planning/coordination/division-of-labor) - not prevent commits by committers because reviewers think it should be done their way instead.

Is there a way we can do that and keep things moving, please?

[dannon]

@carlfeberhard Don't single me out regarding stylistic changes on your pull requests -- I separated this from your pull request and opened it as a separate PR, and merged your existing PR specifically to avoid this preventing of commits that you're worried about. If you look at the other PR, I actually backed up the way you wrote it and said it did work. I did the exact opposite of what you're complaining about.

That said, since we should all be treating this as a completely separate PR and not getting bent out of shape over one that's already been merged -- I'll look at the issues you've raised.

[dannon]

Thanks for the pointer on min -- it was a last minute change when I noticed we were overriding language builtins -- I had only added one of two files and the other change got left out.

[nsoranzo]

@carlfeberhard I've reviewed only the (small) Python part of your PR because I'm not a JavaScript expert.

I cannot see how the review system is preventing your commits: the average time needed to merge your last 10 PRs is around 2 days, which includes the time spent reviewing, discussing and fixing any concern. On the other end, I cannot find any review or comment from you on the last 25 closed PRs from other people.

I'm sorry you do not appreciate the time we spend reviewing other people's PR, in particular how difficult it is to correctly understand their code. Even what you consider "stylistic changes" may reveal bugs or prevent new bugs in the future.
I'm sure others in the project are grateful for the many bugs found by reviewers before their code is merged.

I'm not sure why you two believe I might not think about these things or have reasons for them.

Please use GitHub comments for technical discussion, not personal accusations. I have never thought anything like that. Your code is of the highest quality, but that does not mean it has no bugs or cannot be improved.

[carlfeberhard]

@dannon does this now accept 'limit=None' to load all histories?
If/when it does, I'll +1 and merge. I'm fine with a silent failure on the third point in my comment above. Also apologies if this wasn't the right place to make that request. I feel like this PR is still indicative of an unnecessary change. I appreciate you trying this way, tho (merge original, submit criticism as changes).

[carlfeberhard]

@nsoranzo

TLDR:
It was a professional request - you’re free to say simply say ‘No’ If you'd rather not.

I feel as though the request has only made things more difficult so I apologize (to both of you) for the language. Feel free to do what you feel is right and I’ll do mine. I've tried to make the language in this comment more neutral without completely losing my point/authenticity.

I’ll put this out there: I do not believe this system works - at least not how we’ve implemented it. We only implement the ‘filter-by-committee’ at the end of everyone’s development without any coordination up front (even simple roadmaps) or clear areas of responsibility (a la Mozilla modules, etc.).

Is there some other well-established project out there that has this review system and is making it work? I would like to see how they do it.

Too Short; Give Me More:
This is a technical discussion about our methodology and that was not a personal accusation any more than ‘you do not appreciate the time we spend reviewing other people's PR’. I think most people reading the original conversation would read it as ‘that’s not how he should do that’.

Some people may find stylistic feedback valuable - that’s fine and I don’t speak for them. I have and would to continue to as well if I felt like I could:

• easily say ‘No, I’d rather do it like I have it there’ and not have some large discussion in ‘pull request court’ about the whys and hows of a ternary expression parsing a query string before it got committed
• or easily not say anything without the PR sitting in the queue for days without further comment because I did not ‘address X’s concerns’

re: bugs
Of course I write bugs: I’m conscientious and I work hard - I’m not smart. Again, this wasn’t a bug.

re: my PRs:
2 days because I’ve bumped them along by asking people more often than not.

re: my pr reviews:
When someone submits a PR and:

1. they fall into my (admittedly self-defined) areas of responsibility (my modules, if you will): histories, API, client build, visualizations - in which case, I review and comment
2. it’s not something I’m familiar with and it’s someone who has worked on this project: I read it, trust that they know what they’re doing more than I do, and I do not comment or review if I don’t see/find anything obvious
3. they ask me to look at it - I will, complete with wart check

re: respect
I certainly do value your time, Nicola. I believe we disagree on the role of this review system and the level of detail and subjectivism to be applied.

p.s. how _in the world_ is 2 days reasonable for a commit of ~300 lines?

[dannon]

@carlfeberhard I'm mostly out today, but I'll try to get the extra change pushed up allowing None. Because we'd like to support both "None" and a default with this method, I'll add a simple flag to do that.

Please don't take proposed improvements to code as criticism in any way, this was the point of the separate PR -- they're just that, proposed improvements to the codebase as a whole. You submitted a really nice PR that made mutli-history usable for a lot of people, and that's a great thing. It worked, and I merged it. Along the way, I saw an opportunity to take a method attached to a controller that also used a little bit of special logic to work around an edge case and refactor it; put it in util, expand on it a little to include the ternary case, and make it reusable in other situations. While you're right in that it isn't a 'necessary' functional change to make it work, we do this all the time when refactoring and that's also a good thing.

[dannon]

@carlfeberhard "None" and exception raising implemented in latest commit.

I'm not going to get into it here, but I'd really like for our review system to act as an anti-silo'er. Getting a different set of eyes on your code is the whole point, so please do review code from other areas of the project. Our project is not so large that we can afford to really segment who reviews what.