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

Fixes #9734: Store tags on Directives/Rules #1410

Conversation

ncharles
Copy link
Member

No description provided.

@ncharles
Copy link
Member Author

Commit modified

@ncharles ncharles force-pushed the ust_9734/store_tags_on_directives_rules branch from 0e2901d to 548f168 Compare December 28, 2016 11:30
@ncharles
Copy link
Member Author

Commit modified

@ncharles ncharles force-pushed the ust_9734/store_tags_on_directives_rules branch from 548f168 to bc23eda Compare December 28, 2016 11:41
/**
* We can have multiple Tags with same name - unicity is really on TagName + TagValue
*/
final case class Tags(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need that class ? what is the advantage of having a 'Tags' instead of a 'Set[Tags]' directly in our objects ?

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 don't know what would be the advantage. It is more verbose, and the only possible advantage might be that one day Tags would be different, and so it wouild encapsulate change; but this is dubious
Should I drop it ?

Copy link
Member

Choose a reason for hiding this comment

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

I would have removed it yes!

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, having the Tags class let me define in one location only the rules on unicities - so if we want to change in the future the rules on tags, it'll be easier
so i favour keeping the Tags class

@VinceMacBuche
Copy link
Member

Apart the question on the Tags type, that can be merged !

@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

@Normation-Quality-Assistant Normation-Quality-Assistant merged commit bc23eda into Normation:master Dec 29, 2016
@Normation-Quality-Assistant
Copy link
Contributor

OK, merging this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants