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

chore: remove obsolete flake8 config #2066

Merged
merged 4 commits into from
Jul 1, 2024
Merged

Conversation

afuetterer
Copy link
Contributor

@afuetterer afuetterer commented Jun 24, 2024

What does this PR do?

See #2033 (comment)

Before submitting

  • This PR fixes a typo or improves the docs (if yes, ignore all other checks!).
  • Did you read the contributor guideline?
  • Was this discussed/approved via a Github issue? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes (if applicable)?
  • Did you write any new necessary tests?

@afuetterer
Copy link
Contributor Author

The only configuration setting in the file was the line length. Do you want to use the same setting with ruff?

@MaartenGr
Copy link
Owner

Thanks for the PR! I personally like having a bit longer line length to make certain code a bit more readable (like pandas operations). Having said that, I'm okay either way. Do you have a preference having seen the code in this repo?

@afuetterer afuetterer changed the title chore: remove obsolte flake8 config chore: remove obsolete flake8 config Jun 24, 2024
@afuetterer
Copy link
Contributor Author

afuetterer commented Jun 24, 2024

I prefer longer line length as well. 80 or 88 like PEP8 or black recommend/enforce are too narrow for me.

I usually prefer 100 or 120. 160 might be too long. What do you think?

If you add

[tool.ruff]
line-length = 120

the ruff format --check . step in CI would fail, because ruff would reformat 30+ files.

Its your call, but as you asked me, I suggest 120.

@MaartenGr
Copy link
Owner

I prefer longer line length as well. 80 or 88 like PEP8 or black recommend/enforce are too narrow for me.

Great, I had a feeling I wasn't the only one who liked a bit longer line length here. Let's do 120 since 160 was a bit of a stretch and 100 is too similar to what we have now.

@MaartenGr
Copy link
Owner

Great, LGTM! Should we also re-run ruff to deal with the increased line length or perhaps in a different PR?

@afuetterer
Copy link
Contributor Author

I took the liberty to remove target-version = "py38" as well. As it is unneeded, when requires-python = ">=3.8" is defined. This is recommended by the ruff docs.

Ref: https://docs.astral.sh/ruff/settings/#target-version

@afuetterer
Copy link
Contributor Author

Re-ran ruff, lint job is passing.

@MaartenGr
Copy link
Owner

@afuetterer I just merged a PR that involves simplifying and improving the zero-shot topic modeling approach. It seems that that merge just created a bunch of conflicts for you, sorry!

@afuetterer
Copy link
Contributor Author

Will fix those soon.

@afuetterer
Copy link
Contributor Author

Rebased, re-ran ruff format on changed files.

@MaartenGr
Copy link
Owner

@afuetterer Great, thanks for the work on this!

@MaartenGr MaartenGr merged commit 39bbfdb into MaartenGr:master Jul 1, 2024
6 checks passed
@afuetterer afuetterer deleted the flake8 branch July 1, 2024 14:04
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.

2 participants