-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python3: use open() instead of file() #2370
Conversation
Nice work @nsoranzo! |
@nsoranzo , all: Please stop fixing the 'error' E203? We agreed it was an exception: https://github.com/galaxyproject/galaxy/blob/dev/setup.cfg#L8 Thanks! |
@carlfeberhard If my memory serves, that exception was primarily for dictionary definitions like this:
And I'm totally fine with that, and avoided changing any, as it's arguable they're more readable. That said, consistently using commas like the change at 384be8c#diff-3d1133c72b8beb0cc65d3e0fe244d36aL40 is a good thing, right? |
@carlfeberhard Following on what @dannon said, I tried to fix only places where dictionary definitions were not aligned anyway. |
I'm talking about exactly those types of dictionary changes in this PR: https://github.com/galaxyproject/galaxy/pull/2370/files#diff-546462838b1910ac1552210ce65bf065L122 C'mon, guys: we supposedly agreed on this stuff. Let's spend our energy on testing and docs and not go back and forth on this stuff. |
@nsoranzo Yeah, it looks like you did change some of those towards the bottom of your PR. Did you do this in an automated way? @carlfeberhard So you're good with the other changes, right? I just want to make sure our understanding is clear that the only E203 exception we made was for dictionary definitions, when they're aligned so as to be more readable. We can certainly fix/revert the handful of lines that were erroneously 'fixed' here. Did I miss any in #2364? I was pretty sure all the changes there were solid improvements. |
@dannon Automatically, but reviewed them. In that particular case, it's a dict with 3 entries followed by commented entries which were not aligned, not sure it's worth debating about it further. |
@nsoranzo please don't be dismissive. I haven't '-1'd this PR but I'm within my right to do so. If you don't want to abide by an agreed upon style guide but instead apply your own standards of when/if rules and agreements apply you've ceased to be collaborative. If it's not worth debating it further you could have saved everyone even more time and not made the changes in the first place. |
Yeah, it's definitely worth talking about and getting everyone on the same page. It's not a useful standard if we're not agreed on how to follow it. @carlfeberhard Can you address my questions above? I'm still happy to PR up a reversion of those dict formatting changes. |
I've said it before: Python 3 compat -> awesome! I'm only good with the other changes because the group has voted for us to use an automated script to constrain our python style. I don't personally agree with that. To me, these aren't 'errors' - they're typos that should be 'fixed' when refactoring where you find them - not some script and certainly not some year long project priority. But the group wants to do it - so I'm fine if you want to spend time to do it. Just make sure you're abiding by the agreed upon styles (which you did, @dannon) and try to work with others. |
Hrmm. Yeah, I get fixing these when you run into them -- this all started when I was fixing a bug in #2362 and made this https://github.com/galaxyproject/galaxy/pull/2362/files#diff-df0bd5282b0a1d687b1a51a6253027f9R250 small change. That said, I actually really prefer seeing this type of work separately from substantive changes -- it makes it way easier for me to review tweaks to logic without having to dig through hundreds of lines of junk in the diffs. To each their own, though, on that one. Anyway, to be clear for everyone moving forward -- E203 should not drive changes to dictionary formatting when the previous author has intentionally tried to structure columns for readability. |
Also:
xref. #1715