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

Config output exclude versions #312

Merged
merged 6 commits into from
Dec 15, 2021
Merged

Config output exclude versions #312

merged 6 commits into from
Dec 15, 2021

Conversation

jburel
Copy link
Member

@jburel jburel commented Dec 13, 2021

Exclude keys with version from the output of the config parse command

cc @sbesson @joshmoore

@@ -173,6 +173,8 @@ def get_reference(self):

STOP = "### END"

EXCLUDED_VERSION = ["omero.version"]
Copy link
Member

Choose a reason for hiding this comment

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

What is the different between this new API and BLACK_LIST defined four lines above. Would appending omero.version to the set suffice to exclude this property from being parsed?

(NB: as per our recent review, this might be the occasion to do s/BLACK_LIST/EXCLUDE_LIST

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not want to confuse things but happy to revisit it

Copy link
Member

Choose a reason for hiding this comment

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

If the two exclusion lists achieve different goals, it's fine to introduce new logic. But even then it might be useful to know and document in which context each list should be used when excluding new properties.

Copy link
Member Author

@jburel jburel Dec 15, 2021

Choose a reason for hiding this comment

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

Looking at it closely, they have similar goals so I have added to the list
I have renamed the list. I have not renamed the method but happy to do so

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Let's keep things contained and get this out as a patch release. I will open a follow-up PR to rename the method and deprecate the old one

@sbesson sbesson merged commit 7f875a1 into ome:master Dec 15, 2021
@jburel jburel deleted the config branch May 29, 2024 08:37
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.

3 participants