-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add deprecated-attribute
message
#8857
Add deprecated-attribute
message
#8857
Conversation
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.
Looks pretty good already :) you could install pre-commit locally. A functional test would be nice too see tests/functional/d/ for exampls. Do not hesitate if you have a question.
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.
Nice work. Pushed some changes to move this forward. You should be able to write a functional test now. Let us know how it goes.
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #8857 +/- ##
==========================================
+ Coverage 95.76% 95.77% +0.01%
==========================================
Files 173 173
Lines 18694 18719 +25
==========================================
+ Hits 17903 17929 +26
+ Misses 791 790 -1
|
Tests are now passing (primer is unstable, discussed in #8861) |
I want to ask that do I have to clone astroid as a editable against pylint ? |
Good question! You can, but this PR doesn't need editable astroid. It just needs astroid 3.0.0a8 as listed in the package requirements. |
Yes I was also thinking the same as it get installed with requirements.So would it not fail the primer test i did read the PR that you linked but what i got to know is that you have to exclude the astroid library so can you brief about that ? |
Happy to help, but sorry I'm not understanding the question. You'll notice the unit tests are passing on this PR. The failing jobs are not your responsibility; we'll merge a fix PR and merge main into this branch later. Are you asking about running tests locally? What's the output of |
Maybe I am getting confused then so should I submit a PR withe changes that you mentioned? |
We should continue editing this PR. You can pull these commits from github and then push more commits to this branch, all without creating a new PR. I fixed some of the comments already. |
Oh I have made changes so are there more commits to be done ? |
Does it require more changes ? |
Yes there are unresolved items above that still need to be addressed. |
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.
Logic seems solid and the code looks nice, I think it only lack some functional tests and a proper list created by reading the 3.12 release notes..
I don't think we need more unit tests written as in your screenshot; we would prefer what Pierre mentioned here. You can refer to "functional tests" in the documentation. |
Oh okay no issue |
deprecated-attribute
message
This comment has been minimized.
This comment has been minimized.
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.
LGTM ! I think the fail might be due to the lack of expected message in comment (or maybe the example does not work, not sure why)
Docs check only runs on 3.11, thinking of bumping to 3.12 on this PR |
Co-authored-by: Pierre Sassoulas <pierre.sassoulas@gmail.com>
Or, we can do it in a separate PR so we can rebase here. |
This comment has been minimized.
This comment has been minimized.
We might be stuck if readthedoc is not 3.12 ready yet. |
I can try to find a deprecated attribute in 3.11. |
Actually 3.12 works, but it finds genuine issues with other messages we should solve separately. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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.
Thank you for taking over @jacobtylerwalls !
Description
Closes #8855
I have made changes to 2 files. In deprecated.py, I added a message for the attribute and implemented two related functions. Then, I made changes to the test file of deprecated.py, which is unittest_deprecated.py. I added the function for testing deprecated.py in unittest_deprecated.py.