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
modernization of proselint #1361
base: main
Are you sure you want to change the base?
Conversation
Bumps [ruby/setup-ruby](https://github.com/ruby/setup-ruby) from 1.95.0 to 1.163.0. - [Release notes](https://github.com/ruby/setup-ruby/releases) - [Commits](ruby/setup-ruby@v1.95.0...v1.163.0) --- updated-dependencies: - dependency-name: ruby/setup-ruby dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [codecov/codecov-action](https://github.com/codecov/codecov-action) from 2.1.0 to 3.1.4. - [Release notes](https://github.com/codecov/codecov-action/releases) - [Changelog](https://github.com/codecov/codecov-action/blob/main/CHANGELOG.md) - [Commits](codecov/codecov-action@v2.1.0...v3.1.4) --- updated-dependencies: - dependency-name: codecov/codecov-action dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/setup-python](https://github.com/actions/setup-python) from 2 to 5. - [Release notes](https://github.com/actions/setup-python/releases) - [Commits](actions/setup-python@v2...v5) --- updated-dependencies: - dependency-name: actions/setup-python dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [actions/cache](https://github.com/actions/cache) from 2 to 3. - [Release notes](https://github.com/actions/cache/releases) - [Changelog](https://github.com/actions/cache/blob/main/RELEASES.md) - [Commits](actions/cache@v2...v3) --- updated-dependencies: - dependency-name: actions/cache dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Bumps [pytest](https://github.com/pytest-dev/pytest) from 6.2.5 to 7.4.3. - [Release notes](https://github.com/pytest-dev/pytest/releases) - [Changelog](https://github.com/pytest-dev/pytest/blob/main/CHANGELOG.rst) - [Commits](pytest-dev/pytest@6.2.5...7.4.3) --- updated-dependencies: - dependency-name: pytest dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…che-3 chore(deps): bump actions/cache from 2 to 3
…tup-python-5 chore(deps): bump actions/setup-python from 2 to 5
chore(deps-dev): bump pytest from 6.2.5 to 7.4.3
Bumps [actions/checkout](https://github.com/actions/checkout) from 2 to 4. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v2...v4) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
…eckout-4 chore(deps): bump actions/checkout from 2 to 4
…-ruby-1.163.0 chore(deps): bump ruby/setup-ruby from 1.95.0 to 1.163.0
…decov-action-3.1.4 chore(deps): bump codecov/codecov-action from 2.1.0 to 3.1.4
output was checked for errors
I have managed to rebase everything successfully, and get |
.ruff.toml
Outdated
@@ -1,5 +1,5 @@ | |||
|
|||
#line-length = 90 | |||
line-length = 80 |
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.
can we at least compromise on 88 chars, as it is widely used by black? 80 chars are a relic from the 80's a bit of from the current sweetspot.
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.
88 is a non-standard line length. Sure, 80 characters is an old requirement, but I find you can pretty much always achieve what you need to and it forces you to keep things clean.
It's personal preference, I suppose, but since I refactored proselint the first time it has used 80 character line lengths.
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.
black is the current standard (as it is widely used) in python world - even ruff adopted it and formats the same :)
but ok
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.
Black's completely arbitrary, sarcastically-justified 88-character line length is one of its more controversial "opinions", though.
Particularly as it violates PEP8 — an actual standard, not (at best) a "de facto standard" like Black. (Though PEP8 admittedly did itself no favors by specifying 79 characters instead of 80, something that was met with almost immediate, near-universal rejection.)
IIRC line length was also the very first configuration option Black gained, once its original vision of being completely non-configurable collided headfirst with reality.
proselint/checks/__init__.py
Outdated
if level == "sentence": | ||
pass | ||
elif level == "paragraph": | ||
if level in {"sentence", "paragraph"}: |
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.
thats really wrong, as these are placeholders for custom sub-functionality. not from me, but it had a reason to be like that.
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.
That was a lint suggestion, and it checks out to me. I believe the plan was to add some eventual logic, but I see no reason not to tidy up until then.
.ruff.toml
Outdated
@@ -1,5 +1,5 @@ | |||
|
|||
#line-length = 90 | |||
line-length = 80 |
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.
black is the current standard (as it is widely used) in python world - even ruff adopted it and formats the same :)
but ok
Co-authored-by: Tyler J Russell <xtylerjrx@gmail.com>
Already replaced one of the typeAlias with a named tuple, will also extend the second as it's cleaner. That latest force-push really messes up a lot - I had to do a rebase on my current main (this PR) and hoped /dev would be smoother, but its a hot mess. I have to handle 80+ commits indiviually, deleted files are reappearing randomly.
|
That's good news, thank you.
I know it's not ideal, and I apologise sincerely for that. Thank you for sticking with me and continuing to work on it.
Of course. I'll take a look shortly.
To make this easier, it might be possible to split this into multiple more atomic PRs. I am not sure how you would like to group them, but that would be my suggestion at this stage. I am of course okay with proceeding to review these in whatever structure you like, that's just a comment from my experience.
I would like to at least make progress on the existing PRs around refactoring the configuration system and such, but it might be better to release most of these changes first. Having too many breaking changes in one release could prove hard for people to keep up with. The userbase is, to my knowledge, already dwindling from its former state, so driving more people out would not be ideal. |
Allright. Don't know what PR-splitting brings, but OK. reference: |
I am working on the check categories. I have some basic ideas for now, but more is to follow later. |
[lint.isort] | ||
force-single-line = true |
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.
[lint.isort] | |
force-single-line = true |
I think this should be removed. It makes imports unwieldy.
############################################################################### | ||
# Check-Interface used by linter ############################################## | ||
############################################################################### |
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.
I find large block comments like this to be ugly, so any other method of separation would be preferred.
As a follow-up, you were right. Splitting this PR up would be a nightmare. It would be better to proceed with bringing in your dev branch changes, tidy up the commits here, and then merge this before we work on the other proposals. |
note for myself: it would be useful to consider the discussion in #1369 as part of this, or maybe as a later change. |
Hello,
I put some days into improving the codebase. A lot of open issues should be solved by this. BUT changes are far more substantial as planned - so I wanted to ask how to proceed.
This repo is not very active in the last years. I would offer to maintain the project for the time being and also be open for guiding input. Some of the open Pull Request would be beneficial to the project. I also got plans for new features, improving ease-of-use, more tests for correctness and other optimizations.
I'm open for a chat. Mail is in
setup.cfg
of one of my projects.If there are other plans for the project I would probably fork and start fresh - this is not my preferred solution.
Biggest Changes
Benchmark-Comparison
Windows
Proselint-main (uncached & cached)
Proselint-modernized
Linux / WSL
Proselint-main (uncached & cached)
Proselint-modernized
Breaking changes
--time
->--benchmark
,-b
--debug
->--verbose
,-v
--output-format
controls old flags json, compactWhats broken / untested
Detailed Changelog