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

Add logging configuration to app.py, related tests and Makefile. #23

Merged
merged 4 commits into from Sep 13, 2018

Conversation

ntoll
Copy link
Contributor

@ntoll ntoll commented Sep 12, 2018

Fixes #6.

Heads up!

  • I've moved tests out of the application code.
  • I've added a Makefile (just type make check)
  • I've added coverage reporting on tests.
  • 100% test coverage on new code.
  • Comments in the code should explain intention of what's going on.

Question:

  • Automated code quality checking. In the past I've used pycodestyle and pyflakes. I hear good things about black. I'd like to add code style checking to the automated tests, any opinions on what to use?

I'm not precious about my code, so please don't hesitate to ask for changes! Thanks.

Copy link
Contributor

@redshiftzero redshiftzero left a comment

Choose a reason for hiding this comment

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

This is perfect - thanks @ntoll 😄

CI wasn't building on this branch due to builds of PRs from forks being turned off. I've enabled that now, so on your next push CI should run.

Otherwise in terms of code style, we have flake8 running in CI, but since @kushaldas is a big proponent of black, let's use black 😎 There are just a couple of minor flake8 linting issues that CI would flag in the diff, so let's add black in another commit in this PR prior to merge. And after CI successfully runs on that commit, please go ahead and merge this in! 🎉

@@ -13,4 +13,7 @@ alembic = "*"
[dev-packages]
pytest = "*"
pip-tools = "*"
coverage = "*"
pytest-cov = "*"
pytest-random-order = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

I did not know about this pytest-random-order plugin, looks pretty handy 👀

@kushaldas
Copy link
Contributor

@redshiftzero We will have to use black in the pre-commit hook, there will be nothing to do in the CI.

@ntoll
Copy link
Contributor Author

ntoll commented Sep 13, 2018

Hi folks... so I spent some time looking into black (which I know of but have never used). It's a code-re-formatter rather than code quality checker. It has a --check flag, but this only shows which files it would have reformatted rather than what it would change (AFAICT). Unless I'm missing something here, I believe we want code quality checking rather than code reformatting. Thoughts..?

In any case, I've just noticed Python 3.5... so no f-strings for me. ;-) I'll fix and the CI should pass.

@ntoll
Copy link
Contributor Author

ntoll commented Sep 13, 2018

Also, I added pyflakes and pycodestyle (aka pep8) to the Makefile. If you type make check all the checks, tests and coverage things happen.

@kushaldas
Copy link
Contributor

Hi folks... so I spent some time looking into black (which I know of but have never used). It's a code-re-formatter rather than code quality checker. It has a --check flag, but this only shows which files it would have reformatted rather than what it would change (AFAICT). Unless I'm missing something here, I believe we want code quality checking rather than code reformatting. Thoughts..?

All code should be formatted by black before commit. That is what I was asking for :)

@ntoll
Copy link
Contributor Author

ntoll commented Sep 13, 2018

Hi folks... so I spent some time looking into black (which I know of but have never used). It's a code-re-formatter rather than code quality checker. It has a --check flag, but this only shows which files it would have reformatted rather than what it would change (AFAICT). Unless I'm missing something here, I believe we want code quality checking rather than code reformatting. Thoughts..?

All code should be formatted by black before commit. That is what I was asking for :)

I'm not sure this is a good idea. If code is poetry then black turns everything into a limerick ;-) (N.B. I like limericks, but not all the time).

@ntoll
Copy link
Contributor Author

ntoll commented Sep 13, 2018

There once was a coder called Das,
Who wanted his tests to all pass,
  CI broke and aborted,
  His PR was thwarted,
Said Kushal, "I blame Nicholas".

You're welcome. :-)

@@ -76,4 +76,4 @@ def run():
- create a window for the app.
"""
configure_logging()
logging.info(f'Starting SecureDrop Client {__version__}')
logging.info('Starting SecureDrop Client {}'.format(__version__))
Copy link
Contributor

Choose a reason for hiding this comment

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

I can not tell you how much I want f-strings :(

@ntoll ntoll mentioned this pull request Sep 13, 2018
@redshiftzero
Copy link
Contributor

Looks like black --check in CI would be what we want (and probably also a pre-commit hook) since it returns 1 if code is not formatted according to its standards. That said, since black only supports Python 3.6.0 and later, and we're using Python 3.5 here (due to that being the version of Python we are running right now in the sd-journalist AppVM), let's table that for now, as afaict we'd need to make changes elsewhere if we want to use it.

Otherwise this looks great, mergin'

@redshiftzero redshiftzero merged commit a645f50 into freedomofpress:master Sep 13, 2018
legoktm pushed a commit that referenced this pull request Dec 11, 2023
legoktm pushed a commit that referenced this pull request Dec 15, 2023
Use urljoin instead of string concat for urls.

Thank you for the patch @vivekanand1101 👍
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