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

Adding more methods to Parameters #2028

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

germa89
Copy link
Collaborator

@germa89 germa89 commented Apr 26, 2023

I have been thinking a bit about this.

I personally believe this subclassing is the right way.
And although it is not need because most of the important methods, probably this subclassing can add something else we are missing now. So I would like to go ahead with this.

@germa89 germa89 self-assigned this Apr 26, 2023
@github-actions github-actions bot added Enhancement Improve any current implemented feature New Feature Request or proposal for a new feature labels Apr 26, 2023
@germa89
Copy link
Collaborator Author

germa89 commented Apr 26, 2023

Actually better not.

One thing is the underlying dict _parm which stores the mapping, and another thing is the API to that dict which is this class.

To be fully consistent, inheritate this from dict will imply to get rid of _parm and use the underlying class to store everything.
It can be done, but i don't think it is worth our time right now.

@germa89 germa89 changed the title Subclassing from dict Adding more methods to Parameters Apr 26, 2023
@germa89
Copy link
Collaborator Author

germa89 commented Apr 26, 2023

Repurposing this PR to just add some more methods.

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #2028 (e91ecb7) into main (a25fa9e) will decrease coverage by 0.02%.
The diff coverage is 92.53%.

@@            Coverage Diff             @@
##             main    #2028      +/-   ##
==========================================
- Coverage   82.98%   82.96%   -0.02%     
==========================================
  Files          44       44              
  Lines        7932     7842      -90     
==========================================
- Hits         6582     6506      -76     
+ Misses       1350     1336      -14     

@germa89 germa89 merged commit e2a9546 into main Apr 26, 2023
24 checks passed
@germa89 germa89 deleted the feat/subclassing-parameters-from-dict-class branch April 26, 2023 06:57
@koubaa
Copy link
Contributor

koubaa commented Apr 26, 2023

@germa89 agree with the choice. You can make it feel like a dict without subclassing and that's typically a better choice. The number of methods you have to implement is not that large

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Improve any current implemented feature New Feature Request or proposal for a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants