Skip to content
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

Fix code analysis #206

Merged
merged 4 commits into from Aug 22, 2017
Merged

Fix code analysis #206

merged 4 commits into from Aug 22, 2017

Conversation

gforcada
Copy link
Sponsor Contributor

@gforcada gforcada commented Aug 5, 2017

No description provided.

@gforcada
Copy link
Sponsor Contributor Author

gforcada commented Aug 5, 2017

Regarding the print statements, should we move those to use logging?

One solution could be https://stackoverflow.com/questions/14058453

@mauritsvanrees
Copy link
Sponsor Member

Logging to stdout could help.

On the other hand: why are we against print statements? I think that is because it is not good in a web server environment: all output should go to a log file there, so printing is bad. For a code analysis script on the command line, printing is fine IMHO. So we might disable the printing check for this package.

@gforcada
Copy link
Sponsor Contributor Author

gforcada commented Aug 7, 2017

@mauritsvanrees yeah, that's true as well, I didn't want to have a special set of plugins for p.r.codeanalysis.

The canonical place for plugins anyway should be on bobtemplates.plone or buildout.coredev anyway...

I will remove that plugin at night, thanks for the review!

@gforcada gforcada force-pushed the fix-code-analysis branch 4 times, most recently from a4ffb95 to 7480e1a Compare August 22, 2017 09:51
Done in a breeze thanks to ``add-trailing-comma`` python package!
This way flake8 linters do not complain about an unindexed parameter,
while at the same time the tests keep working.
@gforcada gforcada merged commit 8731f5e into master Aug 22, 2017
@gforcada gforcada deleted the fix-code-analysis branch August 22, 2017 10:00
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.

None yet

3 participants