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

Fixed name collision in class factory definition #345

Closed

Conversation

vhaasteren
Copy link
Member

@vhaasteren vhaasteren commented May 4, 2023

This PR fixes #344. Inside the class factories, the class names now have a trailing underscore. This is not a user-facing change.

The main problem is that if you try to access attributes of the function (the class factory) itself, you get an UnboundLocalError, because the class that has the same name is in this scope and has not been defined yet. Making the names differ fixes that. A reason to access attributes of the class factory would be to get static variables, so one could for instance issue a warning only once, rather than every time you call a class factory. This can be done with global variables also, but it's less neat.

Currently, this PR is not exhaustive.

@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #345 (71caf7e) into master (5ef5ff4) will decrease coverage by 0.01%.
The diff coverage is 93.75%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #345      +/-   ##
==========================================
- Coverage   88.37%   88.37%   -0.01%     
==========================================
  Files          13       13              
  Lines        3012     3010       -2     
==========================================
- Hits         2662     2660       -2     
  Misses        350      350              
Files Changed Coverage Δ
enterprise/signals/parameter.py 84.05% <75.00%> (ø)
enterprise/signals/deterministic_signals.py 100.00% <100.00%> (ø)
enterprise/signals/gp_signals.py 90.68% <100.00%> (ø)
enterprise/signals/selections.py 90.41% <100.00%> (ø)
enterprise/signals/signal_base.py 90.11% <100.00%> (ø)
enterprise/signals/white_signals.py 98.47% <100.00%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ef5ff4...71caf7e. Read the comment docs.

Also changed TimingModel to TimingModel_ in MarginalizingTimingModel
@vhaasteren vhaasteren marked this pull request as draft September 27, 2023 05:46
@vhaasteren vhaasteren changed the base branch from master to dev November 7, 2023 09:01
@vhaasteren
Copy link
Member Author

I'm closing this PR. It's not necessary

@vhaasteren vhaasteren closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Class factories have the same name as the classes they return
1 participant