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

New super #110

Closed
wants to merge 2 commits into from
Closed

New super #110

wants to merge 2 commits into from

Conversation

shimwell
Copy link
Collaborator

This is just an idea to see what can be done about removing some unused arguments from the user view

@shimwell shimwell changed the base branch from main to develop July 23, 2020 16:06
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2020

Codecov Report

Merging #110 into develop will decrease coverage by 0.08%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #110      +/-   ##
===========================================
- Coverage    89.55%   89.47%   -0.09%     
===========================================
  Files           32       32              
  Lines         1944     1948       +4     
===========================================
+ Hits          1741     1743       +2     
- Misses         203      205       +2     
Impacted Files Coverage Δ
...ramak/parametric_components/poloidal_field_coil.py 93.54% <50.00%> (-6.46%) ⬇️

Continue to review full report at Codecov.

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

@billingsley-john
Copy link

Looks great @shimwell. I like the look of this solution.

@shimwell
Copy link
Collaborator Author

shimwell commented Jul 23, 2020

What do people think, is this a better way to do things.

It avoids exposing the users to things they don't want or need.

Any feedback welcome

@RemiTheWarrior @bielsnohr @DeclanMorbey

@RemDelaporteMathurin
Copy link

@shimwell That's super and it simplifies things for users (as well as the docs!)

I reckon these can still be modified in the super class, right ?

Comment on lines 23 to 33
workplane="XZ",
rotation_angle=360,
solid=None,
stp_filename="PoloidalFieldCoil.stp",
color=None,
azimuth_placement_angle=0,
points=None,
name=None,
name='pf_coil',
material_tag="pf_coil_mat",
cut=None,
hash_value=None,
Copy link
Member

@bielsnohr bielsnohr Jul 26, 2020

Choose a reason for hiding this comment

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

The alternative would be to collect these commonly unused arguments into a **kwargs, set the defaults in a dictionary below, and loop through the passed argument to modify those in the default dictionary, which is passed to super.__init__()

Something like (this could definitely be more pythonic, but you get the idea)

default_dict = {...fill in}

for arg in kwargs:
    if arg in default_dict:
        default_dict[arg] = kwargs[arg]

super().__init__(...args you have..., **default_dict)

This leaves open the option to set these at initialisation, but encourages against their use unless absolutely necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion much appreciated, we can give this a go

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have added kwargs to see how it looks :-)

@shimwell
Copy link
Collaborator Author

This can be closed now as the kwarg was implemented in #115

@shimwell shimwell closed this Jul 27, 2020
@shimwell shimwell deleted the new_super branch July 27, 2020 21:27
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.

5 participants