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

Small fixes for flake8 and other smaller pre-commit tools #747

Merged

Conversation

consideRatio
Copy link
Member

@consideRatio consideRatio commented Oct 28, 2021

This PR addresses changes required for the following pre-commit hooks to run properly if we add a .pre-commit-config.yaml later.

  • flake8
  • reorder-python-imports removed on request by @yuvipanda
  • end-of-file-fixer
  • check-executable-have-shebangs
  • prettier (just three lines of changes in .yaml / .md)
  • pyupgrade --py36-plus

I figure applying black would be the real big formatting change, and should be done in a dedicated PR.


I got an error that was really hard to track down by changing the order with requirements-txt-fixer, unsure why. I have excluded that from the PR.

@consideRatio consideRatio changed the title Small fixes for flake8 and other pre-commit tools Small fixes for flake8 and other smaller pre-commit tools Oct 28, 2021
@consideRatio consideRatio force-pushed the pr/pre-adding-pre-commit-fixes branch 2 times, most recently from 500d25a to 4960667 Compare October 28, 2021 09:24
@yuvipanda
Copy link
Collaborator

Overall +1, although can we not use prettier for markdown files? I understand consistency for code files, but feels a little too structured for my personal taste to do that on text.

@consideRatio
Copy link
Member Author

@yuvipanda note that the only markdown file we have in this project at the moment is an issue template that contains ~two actual lines: PULL_REQUEST_TEMPLATE.md

If the docs would be partially or fully MyST markdown parsed in the future, I'd absolutely love to have prettier to do things in the markdown files. From past experience of having MyST markdown + prettier, I value the following from having prettier run on the markdown formatting:

  1. It has catched several python code errors within examples by applying black to them
  2. It has catched several indentation issues in bullet point lists with content under it
  3. It has made me think markdown tables is fine as it take cares of horizontal spacing for the | bars automatically
  4. It has made markdown documentation consistent without being to stubborn in my mind

@manics
Copy link
Member

manics commented Oct 28, 2021

I'm in favour of auto-formatting markdown files too, because it often turns out people do care about the formatting 😄. Line lengths are something I've often noticed- I usually add a line break at the end of sentences but others prefer to insert line breaks based on number of characters in a line.

@consideRatio
Copy link
Member Author

@manics is prettier isn't enforcing a max line length by default in markdown files though, I just verified this in z2jh.

I like that it doesn't, because it is quite messy if you make a code suggestion and suddenly when it is applied you must start fix pre-commit issues or get the automatic commit pushed via pre-commit.ci automation.

@yuvipanda
Copy link
Collaborator

I won't be a blocker for that though :) We can nitpick the markdown rules when they come up!

I'd love for this repo to be on MyST rather than rST as well.

@consideRatio
Copy link
Member Author

@yuvipanda this PR does not introduce the pre-commit configuration, it only removed two spaces in a markdown file based on prettier applied on markdown files.

It is not not clear to me what if anything is requested from me as a PR author

  1. Just make tests pass
  2. Adjust 4960667 to not modify two lines in a markdown file
  3. Update Add .pre-commit-config #748 to not apply prettier to markdown files (which if done, would result in 4960667)

@consideRatio consideRatio force-pushed the pr/pre-adding-pre-commit-fixes branch 2 times, most recently from 60b14cc to a03ca44 Compare October 28, 2021 15:41
bootstrap/bootstrap.py Outdated Show resolved Hide resolved
from tornado.httpclient import HTTPClientError
from tornado.httpclient import HTTPRequest

from tljh.config import CONFIG_DIR
Copy link
Collaborator

Choose a reason for hiding this comment

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

I definitely want us to group imports - is this coming from isort? I don't think black does this.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is coming from reorder-python-imports by asolittle, similar to isort: https://github.com/asottile/reorder_python_imports#readme

It has been used in jupyterhub/jupyterhub, I've grown to appreciate it. Mainly because its groups the stdlib imports, third party imports, and local imports separately. The fact that it makes all imports into separate lines helps a bit with readability in my mind, but I don't care about that difference much.

I've rebased the commit applying changes from it now!

@consideRatio
Copy link
Member Author

consideRatio commented Oct 31, 2021

@yuvipanda I've removed reorder-python-imports (like isort)!

I've also made the pyupgrade --py36-plus hook to run on all files besides the bootstrap/bootstrap.py file that can't yet use f-strings for py35 compatibility. Are you okay with that?

Related PyUpgrade team-compass issue: jupyterhub/team-compass#453, so far I've been happy about changes in jupyterhub and z2jh

@yuvipanda
Copy link
Collaborator

Thank you very much for removing that one, @consideRatio - I think it bothers me immensely, as I have to read through a lot of lines for not as helpful information if they're each split into one line. I wasn't aware it is now in use in JupyterHub, makes me sad :(

@yuvipanda yuvipanda merged commit 770cb3a into jupyterhub:main Nov 1, 2021
@consideRatio
Copy link
Member Author

Haha no worries @yuvipanda!

@yuvipanda
Copy link
Collaborator

As a follow-up, I've opened jupyterhub/ltiauthenticator#74 @consideRatio.

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