MaartenGr / BERTopic

Leveraging BERT and c-TF-IDF to create easily interpretable topics.

https://maartengr.github.io/BERTopic/

MIT License

6.19k stars 765 forks source link

chore: remove obsolete flake8 config #2066

Closed afuetterer closed 4 months ago

afuetterer commented 5 months ago

What does this PR do?

See https://github.com/MaartenGr/BERTopic/pull/2033#issuecomment-2180892902

Before submitting

[ ] This PR fixes a typo or improves the docs (if yes, ignore all other checks!).
[x] 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 commented 5 months ago

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

MaartenGr commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

afuetterer commented 5 months ago

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 commented 5 months ago

Re-ran ruff, lint job is passing.

MaartenGr commented 4 months ago

@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 commented 4 months ago

Will fix those soon.

afuetterer commented 4 months ago

Rebased, re-ran ruff format on changed files.

MaartenGr commented 4 months ago

@afuetterer Great, thanks for the work on this!