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

Refactor Attribute conversion from configuration file and improve documentation. #89

Closed
sallner opened this issue Sep 7, 2020 · 3 comments
Assignees
Labels
documentation Improvements or additions to documentation feedback required gocept
Milestone

Comments

@sallner
Copy link
Contributor

sallner commented Sep 7, 2020

Working with attributes for configuration I stumbled over this line

The default is not passed through the type conversion.

Reading the __get__ method afterwards looks as if the default is actually converted by passing it to __set__

self.__set__(obj, self.default)

Also, multiple places of use show a behaviour which indicate a conversion of the default, e.g.

enable = Attribute("literal", "True")

If you could clarify the situation, I would also create a PR for the docstring chance.

@ctheune
Copy link
Member

ctheune commented Dec 8, 2020

You are right. I'm actually wondering which way would be right. Would you rather like to see defaults parsed or not? Also, it looks like we might have a migration problem or need to consider doing something like: if it's already of the expected type - don't convert it, but specifically non-type conversions would be hard to do ...

@ctheune ctheune added the documentation Improvements or additions to documentation label Dec 8, 2020
@ctheune ctheune changed the title Unlcear documentation of Attribute. Unclear documentation of Attribute. Dec 8, 2020
@ctheune ctheune added this to the batou 2.3 milestone Dec 10, 2020
@ctheune
Copy link
Member

ctheune commented May 31, 2021

So, looking at this in detail we have an issue where we want a conversion function (not just a type factory) that helps crossing the IO boundary between config/secret files and regular Python data structures.

The conversion really should only be happening when handling strings from the config file. Data in Python should be using the proper types already.

I would really appreciate if we reduce the conversion to the explicit cases where we have a config string. To help with documentation it is worthwhile to allow specifying defaults as config strings but making this explicit:

  • no implicit conversions any longer
  • check docs and places where we mention the attributes (at least the Component class)
  • add a from_config_string method to the Attribute class
  • review the locations where we have config strings and explicitly check for attributes that support from_config_string
  • add default_config argument to the Attribute class in addition to default (but XOR their usage) that will be run through the conversion

@sallner sallner changed the title Unclear documentation of Attribute. Refactor Attribute conversion from configuration file and improve documentation. Jul 9, 2021
ctheune added a commit that referenced this issue Oct 5, 2021
 re #89: Avoid implicit conversion of Attribute defaults.
@sallner
Copy link
Contributor Author

sallner commented Oct 8, 2021

Should be fixed with #200, can be closed now.

@sallner sallner closed this as completed Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feedback required gocept
Projects
None yet
Development

No branches or pull requests

2 participants