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

[deprecated-argument] Create the initial documentation #8995

Merged

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
βœ“ πŸ“œ Docs

Description

Refs #5953

@codecov
Copy link

codecov bot commented Sep 2, 2023

Codecov Report

Merging #8995 (18f4055) into main (fb2faee) will decrease coverage by 0.02%.
Report is 8 commits behind head on main.
The diff coverage is 57.14%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #8995      +/-   ##
==========================================
- Coverage   95.76%   95.75%   -0.02%     
==========================================
  Files         173      173              
  Lines       18639    18645       +6     
==========================================
+ Hits        17850    17853       +3     
- Misses        789      792       +3     
Files Changed Coverage Ξ”
pylint/testutils/_primer/package_to_lint.py 73.23% <40.00%> (-2.52%) ⬇️
pylint/checkers/base_checker.py 95.04% <100.00%> (+0.04%) ⬆️


def deprecated_arguments(
self, method: str
) -> tuple[tuple[int | None, str], ...] | tuple[tuple[int, str], tuple[int, str]]:
Copy link
Collaborator

@matusvalo matusvalo Sep 3, 2023

Choose a reason for hiding this comment

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

I think an example is not clear out of blue. I needed to go back to source code documentation in order to understand it. We should put here a link to documentation of parameters and return value (or at least put a link to an example https://github.com/pylint-dev/pylint/blob/main/examples/deprecation_checker.py) or document it right here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot about this example, how about only a link to the example ? I also was not very clear how it should be done. Do we agree that the only way is to create a plugin, there's no configuration that can make deprecate-arguments be raised otherwise, right ?

Copy link
Member

Choose a reason for hiding this comment

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

there's no configuration that can make deprecate-arguments be raised otherwise, right ?

I think I'm missing some context here, what is the reason we can't just have a vanilla example that uses one of the deprecated arguments in stdlib.py?

Copy link
Member Author

@Pierre-Sassoulas Pierre-Sassoulas Sep 5, 2023

Choose a reason for hiding this comment

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

No I thougth the same. But after a 10mn search I found no such example of deprecated arguments in the stdlib (?!). Chatgpt did hallucinate one though :D Also it seems that we only have a base class that permit to create a custom checker and not an actual builtin pylint checker ? (I did not find anything in pylint either). This is what I'm trying to confirm with @matusvalo in fact :)

Copy link
Member

Choose a reason for hiding this comment

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

πŸ‘€

(3, 9, 0): {"random.Random.shuffle": ((1, "random"),)},
(3, 12, 0): {
"coroutine.throw": ((1, "value"), (2, "traceback")),
"shutil.rmtree": ((2, "onerror"),),

Copy link
Member Author

Choose a reason for hiding this comment

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

Weeeell, I just feel silly now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, I'm wondering if the custom checker could now be replaced by an option using qname in order to add value in this data structure after the fact ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now I am not sure whether everything was answered or there is still something waiting for me... :-D

Copy link
Member Author

Choose a reason for hiding this comment

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

Any idea why we can't add values in DEPRECATED_ARGUMENTS via options/configuration instead of creating a custom checker @matusvalo ? Thank you for initially reviewing the PR :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea why we can't add values in DEPRECATED_ARGUMENTS via options/configuration instead of creating a custom checker @matusvalo ?

It depends on the scope I suppose. If the scope is just python I think sure it can be via configuration I suppose. But if you want to have it as generic checker to be used for other projects - e.g. for django the plugin developer needs to specify logic how you get framework version in order to compare it to the configuration.

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the deprecated-argument-doc branch 2 times, most recently from 7103f94 to 48c67ea Compare September 4, 2023 13:33
@Pierre-Sassoulas
Copy link
Member Author

I added an example with int because depending on the version we can have syntax error once the deprecated arguments is removed. But we could add another example using py-version possibly.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 20fa209 into pylint-dev:main Sep 7, 2023
@Pierre-Sassoulas Pierre-Sassoulas deleted the deprecated-argument-doc branch September 7, 2023 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation πŸ“— Skip news πŸ”‡ This change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants