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
Black Formatter #769
Black Formatter #769
Conversation
Thanks for the PR. I'll review this on Tuesday (my next jrnl work day). Small point of order: the contributing guidelines ask that we open a new issue for new features. This gives us a chance to talk about things before you put the work into implementing it. In this case, I do happen to like Black already, but please try to follow the guidelines in the future. |
I created issue #770 as a place to have some discussion around this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Just need a bit of cleaning up of the travis config.
Remove "Lint" as separate stage; have `black` check the output rather than run the re-formmater
Hey, this is going to be very prone to merge conflicts, and I don't want you to try to hit a moving target. Can you take out the changes made by black itself, and just do the travis file and the |
It is otherwise ready for approval, though. Thank you! |
# Conflicts: # jrnl/EncryptedJournal.py # jrnl/Journal.py # jrnl/__init__.py # jrnl/cli.py # jrnl/install.py
@wren I just updated it to the current 'develop' branch. Mostly as a way to get Travis-CI to run (maybe..). It should be a clean merge at the moment. |
Black doesn't want to make any changes locally, but want to on Travis, so I added the |
|
||
|
||
if type(journal_conf) is dict: | ||
# We can override the default config on a by-journal basis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# We can override the default config on a by-journal basis | |
# We can override the default config on a by-journal basis |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving so I can get this merged and fix the issues.
@MinchinWeb The problem, I'm guessing, is that you're running a different version of black than the build is on Travis. Make sure you're using poetry (e.g. poetry run black .
) when running locally to ensure the same version.
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This adds the
black
formatter as Linting stage to the Travis-CI setup. The existing code was also adjusted, as needed, to pass the linter.This linting stage has to pass before the regular tests are run.
Checklist