Feature: restructure Parameter and Metadata and create non-saved input-fields

[CasperSchippers]

A suggested implemented for #1014 (and PR #1013).

[codecov]

Codecov Report

Attention: Patch coverage is 80.00000% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 58.32%. Comparing base (2ad067b) to head (c504aa0).

Files	Patch %	Lines
pymeasure/experiment/parameters.py	76.47%	20 Missing ⚠️
pymeasure/experiment/procedure.py	80.76%	5 Missing ⚠️
pymeasure/display/widgets/filename_widget.py	0.00%	1 Missing ⚠️
pymeasure/display/widgets/inputs_widget.py	95.23%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1036      +/-   ##
==========================================
+ Coverage   58.26%   58.32%   +0.05%     
==========================================
  Files         258      258              
  Lines       17874    17901      +27     
==========================================
+ Hits        10414    10440      +26     
- Misses       7460     7461       +1     

Flag	Coverage Δ
unittests	58.32% <80.00%> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[CasperSchippers]

@BenediktBurger, @bilderbuchi, and/or @msmttchr: what do you think of a structure like what I proposed here?

I'm not sure now what I think of having all these classes (a lot of which are just inheritances without any changes, just to be able to 'dispatch' on the class-type). Maybe it's fine like this, but am not sure yet, so I'm curious what you think of this structure and, if you don't like it, if you have any other/better suggestions?

[BenediktBurger]

A few comments (mainly regarding documentation typos).

I see the introduction of many very similar classes, just different in type.

Another idea: what about a "type" parameter for the Input Field?
Valid arguments could be native types (float, int...) or an enum member (especially for those additional types).
So it would be steps = InputField(type=int) (or Parameter or Metadata instead of InputField) or type=Types.INT using an enum.

That is easy, but reduces the number of classes drastically.

[BenediktBurger]

Suggested change

	writen to the data-file, and whether the value is applied when getting parameters from a file.
	written to the data-file, and whether the value is applied when getting parameters from a file.

[BenediktBurger]

Suggested change

	""" Returns True if the InputField value is set
	""" Return True if the InputField value is set

[BenediktBurger]

Suggested change

	The data dat is stored in a parameter is written to the parameters section of the data file
	The data that is stored in a parameter is written to the parameters section of the data file

[BenediktBurger]

Is it necessary to state the parent class?

In the api documentation you can toggle "show_inheritance" to show that information.

That makes the actual inheritance the single point of truth.

[BenediktBurger]

Why do you have lengthy docstrings for the parameters, but not for the Metadata?

Naive, I would not add docstrings to either one.

[CasperSchippers]

I simply did not get around to doing that. I was not yet completely finished with this, but mainly wondered about the structure with all the classes.

I would be inclined to add docstrings since the classes are handled differently, so I think, for clarity, it would be good to mention their purpose and how they are handled in the docstrings.

[CasperSchippers]

Another idea: what about a "type" parameter for the Input Field?

I chose the classes approach for symmetry to the existing set of Parameter classes (IntParameter, FloatParameter and such).

Hmmm, that is maybe cleaner. However, I do not want to break backwards compatibility, so I'm not sure if we want to remove the parameter classes.

An option would be to rewrite it with the types as you suggest, and add the set of Parameter classes for backwards compatibility?

[BenediktBurger]

An option would be to rewrite it with the types as you suggest, and add the set of Parameter classes for backwards compatibility?

Reading the email, I wanted to suggest exactly that, just to note, that you added it yourself 😁

[CasperSchippers]

Mainly as a note to myself (though feel free to criticize)

One challenge I saw with having a single parameter class was the fact that all the different subclasses now have custom convert, __str__, and __repr__ methods.
One way to solve this is to create gigantic versions of that method with some if-else switch statement (match-case could be a better fit but is nog introduced in 3.10) to effectively dispatch between different versions; seems ugly though.

A solution might be composition: have a Parameter class, which is effectively a type agnostic container, but holds a Value class that serves the different types. The convert, __str__ and __repr__ of Parameter would simply pass through to this Value class. Parameter would also have some flags, like write_to_file, load_from_file and is_metadata.

If going this way, I'm also inclined to not make string the 'default'/base class but rather have an abstract base, and string just one of the specific instances.

I am starting to wonder now if this is not an overly complicated solution for something simple... Maybe the original proposal #1013 is cleaner after all...???

[bilderbuchi]

Composition over inheritance is often recommended in principle, so the sketched approach sounds reasonable to me. 👍

I can't judge the relative merits of the other proposal, I've never been deeply immersed in that part of the codebase.

[BenediktBurger]

A solution might be composition: have a Parameter class, which is effectively a type agnostic container, but holds a Value class that serves the different types. The convert, __str__ and __repr__ of Parameter would simply pass through to this Value class. Parameter would also have some flags, like write_to_file, load_from_file and is_metadata.

I like that solution in favor of complicated methods.

If going this way, I'm also inclined to not make string the 'default'/base class but rather have an abstract base, and string just one of the specific instances.

Sounds good.

I can't judge the relative merits of the other proposal, I've never been deeply immersed in that part of the codebase.

The final decision on the approach you have to do yourself, maybe with @msmttchr, as neither I am an expert in that part of the codebase