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 Section properties cardinality #383

Merged
merged 10 commits into from
Apr 16, 2020
Merged

Conversation

mpsonntag
Copy link
Contributor

The current PR adds the basic cardinality implementation for Section.properties. It provides

  • an update in the Section.init.
  • prop_cardinality accessor methods.
  • a set_properties_cardinality convenience method.
  • an update to the tools.dict_parser to properly write and read tuples with yaml.
  • and registers a Section properties cardinality validation.
  • tests for the implemented features.

The PR refers to issue #361. Another PR providing the missing Section.sections cardinality will follow.

Adds and registers a validation to check the properties
cardinality of Sections.
Adds tests for Section set properties cardinality
including a general function that can be reused for
Section set sections cardinality tests.
Adds tests for the Section properties cardinality
convenience set method including a general method
that can be reused for the Section sections cardinality
convenience set method.
Tests saving and loading the Section properties cardinality
attribute to and from all supported file formats.

Adds a general test method that can be reused with the
Section sections cardinality.
Tests the Section properties cardinality validation
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 76.629% when pulling fa2dd57 on mpsonntag:cardSecA into eccae1f on G-Node:master.

Copy link
Member

@jgrewe jgrewe left a comment

Choose a reason for hiding this comment

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

I just wondered if we shouldn't extend this functionality to constrain based not only on the number of properties, but also on the property names? E.g. there must be only one property named amplifier, or there must be at least one experimenter and one date property...

@achilleas-k
Copy link
Member

Don't we restrict name collisions anyway? At least when validating.

@jgrewe
Copy link
Member

jgrewe commented Apr 16, 2020

ah, true... You are right, I was thinking into the "type" direction which does not apply for properties.

@achilleas-k
Copy link
Member

I was thinking it might be useful to have restrictions on types for subsections, but I think that might complicate things, at least for now. Might be good to think about though.

@achilleas-k
Copy link
Member

achilleas-k commented Apr 16, 2020

Discussions aside, we got two approvals, so I'm merging. We can open issues for the other ideas.

@achilleas-k achilleas-k merged commit 13fd740 into G-Node:master Apr 16, 2020
@mpsonntag mpsonntag deleted the cardSecA branch April 17, 2020 09:55
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.

4 participants