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

added pylint config for linting Python code #6473

Closed
wants to merge 3 commits into from

Conversation

alexvoss
Copy link
Sponsor Collaborator

These are the changes that would be needed to integrate pylint as suggested in #6472

I accidentally also added a bump of ESLint to version 6.13.1. I saw dependabot suggest this but the PR being rejected, so perhaps there is a reason why this is not a good idea (yet). What prompted me to bump this was that I got deprecation warnings with the older version, complaining that the newer version of Typescript being used was unsupported.

@alexvoss
Copy link
Sponsor Collaborator Author

On the choice of Pylint: it says about itself that it is "not afraid of being a little slower than it already is, [it] is also a lot more thorough than other linters. There are more checks, including some opinionated ones that are deactivated by default but can be enabled using configuration." It also suggests to use other linters alongside it, which I think is just sensible.

Especially people who have a slower machine may well want to use in ruff in their IDE. I would still run Pylint in the delivery pipeline. So, not so much a question of either or.

The changes needed here are tiny and the impact should be tangible, though I agree that they benefit end users only indirectly.

Btw. I will take the ESLint bump out again because it seems to have triggered a deprecation warning (see failed check in the pipeline) and so probably best to deal with it separately.

@squidfunk
Copy link
Owner

Yeah, I need to update the linters, there are some pinned dependencies because some rules have been phased out. Looks a little like moving forward for the sake of moving forward, so I considered this to be a low priority. As said, linting is good, but it is certainly not the most important thing we should be working on. Let me finish the projects plugin rewrite, and then let's have a call to discuss how we can start integrating linting and maybe auto-formatting gradually.

@alexvoss
Copy link
Sponsor Collaborator Author

moving forward for the sake of moving forward

I think it is more than that. We have just documented PRs and IMHO as part of that we should a) describe the linters already in use and b) add a Python one since there is a fair bit of Python code.

Other things may be more pressing right now but I think we should do this pretty soon since it will pay you back every day once done, especially if we get more PRs as a result of having explained the process - unless the documentation has the opposite effect or putting people off. (I hope it has the effect of encouraging quality PRs while preventing some of the low quality ones.)

@squidfunk
Copy link
Owner

squidfunk commented Dec 10, 2023

With "moving foward for the sake of moving forward" I meant that specific rules have been removed from Stylelint and ESLint, which is why I had to pin these dependencies.

I absolutely agree on improving the environment for outside collaborators. However, since I'm currently doing 99% of changes on the code base, introducing new processes must be done carefully, as I'm the one that is most impacted by those changes. Since I'm currently super busy pushing this project forward, adding new tooling to the build chain is something I'll be very, very selective about. Once the dust settles around search and the projects plugin, we can definitely consider moving forward here. Until then, it is not a priority. The toolchain currently works very well, and yes, we need to improve it, but please let's push this a little into the future. I suggest mid to end of Q1 2024. I hope that's okay for you.

As a corollary: I also wouldn't get a new notebook 1 day before an important presentation.

and that is used by both the python job and npm-check. This way we do
not need to repeat ourselves.
@squidfunk
Copy link
Owner

squidfunk commented Apr 27, 2024

We're currently not considering this change, but we're planning to do a major restructuring of our Python code base later this year or next year (depending on how much other things are on my/our plate). Let's close it for now, and then reconsider. I've created a memo in the backlog of our internal project board.

@squidfunk squidfunk closed this Apr 27, 2024
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

2 participants