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

Always use __dict__ and ignore __slots__ on classes #169

Closed
wants to merge 2 commits into from

Conversation

Nodd
Copy link
Contributor

@Nodd Nodd commented Jun 3, 2023

Fix #168

@Nodd
Copy link
Contributor Author

Nodd commented Jun 3, 2023

The workflow failure seems unrelated to my modification.

@codecov
Copy link

codecov bot commented Jun 3, 2023

Codecov Report

Merging #169 (6bbddc0) into main (ae69f55) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #169      +/-   ##
==========================================
+ Coverage   89.83%   89.88%   +0.05%     
==========================================
  Files          27       27              
  Lines        1141     1137       -4     
==========================================
- Hits         1025     1022       -3     
+ Misses        116      115       -1     
Impacted Files Coverage Δ
sphinx_automodapi/automodsumm.py 85.84% <100.00%> (+0.13%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pllim
Copy link
Member

pllim commented Jun 3, 2023

This is related though.

sphinx_automodapi/automodsumm.py:86:1: F401 'abc' imported but unused
1

I'll have to ping @eteq , @jairideout , and @pkgw based on the blame on the affected function.

@pllim
Copy link
Member

pllim commented Jun 3, 2023

RTD failure is indeed unrelated. I don't know why it is pulling such old versions.

@Nodd
Copy link
Contributor Author

Nodd commented Jun 3, 2023

This is related though.

sphinx_automodapi/automodsumm.py:86:1: F401 'abc' imported but unused
1

I fixed this, but there was another error : Reason: UndefinedError("'style' is undefined")

I'll have to ping @eteq , @jairideout , and @pkgw based on the blame on the affected function.

Yeah, I guess the added complexity was there for a reason ? But if include_base is True there is no special handling of __slots__ and everything works correctly in my case.

@pllim
Copy link
Member

pllim commented Jun 3, 2023

@Nodd , unfortunately I wasn't involved in the original design of the code, so I have no idea what it is trying to do, but hopefully one of the contributors could comment more. 🤞

I am trying to fix fixed RTD over at #170. If that works, I'll merge it and then you can rebase to pick up the fix. 🤞 🤞

Thanks for your contribution and for your patience!

@Nodd
Copy link
Contributor Author

Nodd commented Jun 3, 2023

Looks like the rebase went well!

Copy link
Member

@bsipocz bsipocz left a comment

Choose a reason for hiding this comment

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

This could use a regression test, could you add the one you reported in #168?

Also, will need a changelog entry.

@Nodd
Copy link
Contributor Author

Nodd commented Jun 15, 2023

OK, I'll do this as soon as I can find some time.

@kylefawcett
Copy link
Contributor

kylefawcett commented Dec 19, 2023

I ran into this problem as well when attempting to automodapi a module containing some classes that extend pydantic's BaseModel which uses __slots__. I see that @Nodd has the fix in this PR but it appears to be waiting for unit testing. To help this along, I'd like to contribute some test I've implemented here.

@Nodd can cherry pick the commit with the tests, or I can create a new PR. No preference on my end.

@pllim
Copy link
Member

pllim commented Dec 19, 2023

Thanks for your interested in this PR! Since it has been 6 months, if a new PR could be made with cherry picking the original commits here + new stuff needed to push it over the finish line, that would be great! 🙇‍♀️

@Nodd
Copy link
Contributor Author

Nodd commented Dec 19, 2023

Sorry, I never found the time to finish this. @kylefawcett you can create a new PR, I'll be very happy if you manage to get it done 😃

@kylefawcett
Copy link
Contributor

@pllim -- Will do.
@Nodd -- sounds good. I've cherry picked the commits with your fix as is so you'll still be in the commit history. I added my test cases on top of that in a new branch which will be submitted as a new PR soon.

@pllim
Copy link
Member

pllim commented Dec 19, 2023

Thanks, all! Superseded by #181

@pllim pllim closed this Dec 19, 2023
bsipocz added a commit that referenced this pull request Jan 27, 2024
Updated "Use __dict__ and ignore __slots__ on classes #169
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.

Using __slots__ hides class variables
4 participants