Skip to content

[STY] PEP8 style changes and minor logging fixes#253

Merged
josephmje merged 3 commits intoTIGRLab:masterfrom
josephmje:master
Jan 3, 2020
Merged

[STY] PEP8 style changes and minor logging fixes#253
josephmje merged 3 commits intoTIGRLab:masterfrom
josephmje:master

Conversation

@josephmje
Copy link
Copy Markdown
Contributor

No description provided.

@auto-assign auto-assign bot requested review from DESm1th, edickie and jerdra January 2, 2020 18:38
Comment thread datman/xnat.py Outdated

def get_sessions(self, study):
logger.debug('Querying xnat server for sessions in study'
logger.debug('Querying xnat server for sessions in study:{}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's not a giant hassle (or a difference in preferred style) could you throw a space after the colons in these messages? I dont know about you but all our old log messages that leave the space out drive me a bit crazy haha

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

@DESm1th
Copy link
Copy Markdown
Contributor

DESm1th commented Jan 2, 2020

Thanks for catching all these issues!

@jerdra
Copy link
Copy Markdown
Contributor

jerdra commented Jan 2, 2020

looks good to me, thanks for catching!
edit: just realized you have yet to make changes, I'll re-approve when finished!

jerdra
jerdra previously approved these changes Jan 2, 2020
@pep8speaks
Copy link
Copy Markdown

pep8speaks commented Jan 2, 2020

Hello @josephmje! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 179:80: E501 line too long (80 > 79 characters)
Line 517:80: E501 line too long (80 > 79 characters)

Comment last updated at 2020-01-02 21:38:10 UTC

@jerdra
Copy link
Copy Markdown
Contributor

jerdra commented Jan 2, 2020

will approve after pep8 changes!

@josephmje
Copy link
Copy Markdown
Contributor Author

josephmje commented Jan 2, 2020

will approve after pep8 changes!

i was going to ask whether we should increase the line length?
black defaults to 88 characters. we can also use B950 so we only get flagged if the length length is exceeded by more than 10%

@DESm1th
Copy link
Copy Markdown
Contributor

DESm1th commented Jan 3, 2020

Hmm.. I personally dont have any strong preferences either way. But I will say that python's actual line length limit lets me fit two tabs side by side on that lab monitors without missing the end of lines or having to close atom's project pane haha. I can definitely work around that though and will just go with whatever you guys prefer

@jerdra
Copy link
Copy Markdown
Contributor

jerdra commented Jan 3, 2020

Agree with @DESm1th on editor usability.
Messing around with line lengths will always be weird to deal with 🤷‍♂️ since you kind of want to always add that extra bracket over the end.
Maybe we'll just exercise judgement on whether we think a particular flake8 issue is problematic?

For example, for the current pep8 issues I'd be totally cool with just letting it pass since it's just a bracket. Only issue is that this will stay persistent on future updates to the code 😞

@DESm1th
Copy link
Copy Markdown
Contributor

DESm1th commented Jan 3, 2020

^ All of that sounds incredibly reasonable to me (including letting the bracket pass).

@jerdra
Copy link
Copy Markdown
Contributor

jerdra commented Jan 3, 2020

Okkkk Dawn and I had a mini-chat:
We propose the following:

Use 10% feature on the condition that all auto-code formatters should stick to 80 characters. Dawn and I have been using yapf for our code-formatting which we're mostly happy with.

That way any violation of 80 characters is intentional and made with full awareness

@josephmje
Copy link
Copy Markdown
Contributor Author

Cool! I'm adding some extra configuration to flake8 in my WIP PR. Will keep line length at 80 and add B950.

@josephmje josephmje merged commit 94c258d into TIGRLab:master Jan 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants