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

fix: reporting the attribute who lost their value #225

Merged
merged 5 commits into from
Nov 11, 2023

Conversation

gpuligundla
Copy link
Contributor

This PR Address the #218 issue.

Describe the bug
For example, going from:

class A:
    b: int | None = None

to

class A:
    b: int

will report

Attribute value was changed: None -> None

Now modified to

Attribute value was changed: None -> unset

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks!

It would be better to pass "unset" to the breakage instance rather than modifying the value in the actual attribute 🙂

@gpuligundla
Copy link
Contributor Author

Thanks!

It would be better to pass "unset" to the breakage instance rather than modifying the value in the actual attribute 🙂

Changed it

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

Thanks. I realise it needs an else clause, otherwise it would yield both breakages.
Could you also add a test :) ?

@gpuligundla
Copy link
Contributor Author

Thanks. I realise it needs an else clause, otherwise it would yield both breakages. Could you also add a test :) ?

ah! yeah sorry for being noob, i assumed it as return keyword

Copy link
Member

@pawamoy pawamoy left a comment

Choose a reason for hiding this comment

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

No worries! And thanks for the update 🙂 LGTM now, merging!

@pawamoy pawamoy merged commit dfffa4b into mkdocstrings:main Nov 11, 2023
16 checks passed
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.

2 participants