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

Use the default config in the messages documentation tests #7142

Merged
merged 1 commit into from
Jul 7, 2022

Conversation

DanielNoord
Copy link
Collaborator

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
🔨 Refactoring

Description

Will unblock #7070 and others as it changes the value for max-return-values from our own 11 to the default of 6.

@DanielNoord DanielNoord added Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow labels Jul 7, 2022
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

That's already a great change, but how about we also show the configuration file in the documentation ? That way :

  • it's immediately obvious what need to be done to activate the check if it's an extension, or a standard but optional check (soon ™️).
  • We can create simpler example by lowering the values to something sane (i.e. max line length = 15 instead of having 120+ character in the example, max-return-values = 2 ...)
  • While at the same time demonstrating some options that can affect the message (!)

What do you think ?

@DanielNoord
Copy link
Collaborator Author

That's already a great change, but how about we also show the configuration file in the documentation ? That way :

This can be seen by looking at the examples folder. We would just duplicate the files there.

  • We can create simpler example by lowering the values to something sane (i.e. max line length = 15 instead of having 120+ character in the example, max-return-values = 2)

You can use a dedicated pylintrc file for this right?

Comment on lines +1 to +2
[MAIN]
load-plugins = pylint.extensions.bad_builtin
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We already have:
This message is emitted by the optional 'deprecated_builtins' checker which requires the pylint.extensions.bad_builtin plugin to be loaded.
on that page?

Copy link
Member

Choose a reason for hiding this comment

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

And then we could add:

bad-functions = ["map", "filter", "something_custom"]

to this, and demonstrate very succinctly how to add new bad builtins.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let's do that in another PR. There are a lot of moving pieces with that as well:
How to identify related options? How to show the difference between the default value and the value being used in a test? How to handle duplication of the load-plugins warning.

I really intended this PR to unblock some of the PRs that do have correct examples.

Copy link
Member

Choose a reason for hiding this comment

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

We already have: This message is emitted by the optional ...

Sure but that's text, and there's two click to reach the possible options. Code that is guaranteed to work alongside with an example is a lot nicer imo.

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

Sure ! Thank you for this, great PR that open a lot of possibilities.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2628103770

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.359%

Totals Coverage Status
Change from base Build 2625569561: 0.0%
Covered Lines: 16705
Relevant Lines: 17518

💛 - Coveralls

@Pierre-Sassoulas
Copy link
Member

How to identify related options?

Yeah, that's a problem for us and for user alike 😄

@DanielNoord DanielNoord merged commit f465dd8 into pylint-dev:main Jul 7, 2022
@DanielNoord DanielNoord deleted the docs branch July 7, 2022 08:54
@DanielNoord
Copy link
Collaborator Author

@Pierre-Sassoulas Can you turn on the update branch button on this repository like I have done for pydocstringformatter? It allows easily updating the affected PRs from within the Github interface without pulling them locally.

@Pierre-Sassoulas
Copy link
Member

Opened #7143

Can you turn on the update branch button on this repository like I have done for pydocstringformatter?

Sure ! (And I'm still trying to give you the right to do that :) )

@Pierre-Sassoulas
Copy link
Member

The option is:

Always suggest updating pull request branches
Whenever there are new changes available in the base branch, present an “update branch” option in the pull request.

But I think it makes it mandatory and we ruled against this because there is a very high number of commit in pylint each day and being forced to update to the latest main was being very harsh on the github actions runners for often no reasons (it's not often that we have incompatible branch). I agree that having to fix a conflict or add a remote locally is very annoying. What do you think ?

@DanielNoord
Copy link
Collaborator Author

Ah yeah, probably best to keep it off. Too bad..

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Jul 7, 2022

For what it's worth I contemplated voluntarily creating conflicts with the online editor so it's possible to merge main from the interface 😄 But now that I'm used to adding remote and removing them right after it's isn't that bad. But it's annoying to not have an option to do that without it being mandatory.

@DanielNoord
Copy link
Collaborator Author

gh pr checkout 7142 works quite well and doesn't add to remote I think.

That said, sometimes I want to fix/merge a PR from the browser and I don't think you can pull/merge main in the online editor (yet).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation 📗 Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants