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

Extend "optional" flag for ConfZFileSource to ignore more exceptions #53

Merged

Conversation

ajanon
Copy link
Contributor

@ajanon ajanon commented Jul 6, 2022

See PR #50 and issue #52.

optional now makes it possible to ignore if a command line argument is not set (through file_from_cl) or if an environment variable is not present (file_from_env).

See PR Zuehlke#50 and issue Zuehlke#52.
"optional" now makes it possible to ignore if a command line argument is not
set (through "file_from_cl") or if an environment variable is not
present ("file_from_env"). If set, this also ignores errors if the
file format is invalid.
@silvanmelchior
Copy link
Collaborator

Thanks a lot for your PR!

One small thing: I wouldn't allow _get_format to fail, as this does probably not reflect the intention of the user using the optional flag.

@ajanon
Copy link
Contributor Author

ajanon commented Jul 6, 2022

That makes sense!
It does look kinda ugly, though:

try:
    file_path = cls._get_filename(confz_source)
except ConfZFileException as e:
    if confz_source.optional:
        return
    raise e
file_format = cls._get_format(file_path, confz_source.format)
try:
    file_content = cls._read_file(file_path, file_format, confz_source.encoding)
except ConfZFileException as e:
    if confz_source.optional:
        return
    raise e

Do you think this could be written better? The order of the operations needs to stay the same, and I cannot see any other way to have get_format outside the block. I could also check inside each function, but as mentioned in the issue, this may not be more maintainable.
If this is fine with you, I will update this PR accordingly.

@silvanmelchior
Copy link
Collaborator

True, it looks a bit ugly, but I don't see a good alternative. A context manager which would catch and optionally ignore the error like with AllowException(ConfZFileException, enabled=optional): ... does not help as we would still need to change the control flow (no follow-up calls if the first one fails). So I think we can go with your proposed solution.

The previous change made the "optional" flag also ignore file formats related
exceptions. As this may be confusing for the user, this commit reestablishes the
previous behavior for file formats: an error will still be generated on
incorrect values, regardless of the "optional" flag.
@silvanmelchior
Copy link
Collaborator

Great, thanks! I will merge and release a new version tmr.

@silvanmelchior silvanmelchior merged commit d028268 into Zuehlke:main Jul 7, 2022
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.

Make file_from_cl command line argument and file_from_env environment variable optional
2 participants