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

Feature/xand tag #1

Merged
merged 6 commits into from
Aug 6, 2024
Merged

Feature/xand tag #1

merged 6 commits into from
Aug 6, 2024

Conversation

tinytyranid
Copy link
Owner

I have added the possibility to add a xand tag.

It works similarly to the xor tag, but requires all flags within the xand group to be set.

I have tried to stay consistent to the style already used, and touch existing functions as little as possible.

@tinytyranid tinytyranid self-assigned this Aug 2, 2024
Copy link
Collaborator

@Stautis Stautis left a comment

Choose a reason for hiding this comment

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

I think the feature makes sense and your implementation looks good ☺️ you're piggybacking on some functions, but I think that makes sense since the Xand and the Xor flags are logically related and your implementation style is pretty similar to the one already used. Good job, Camilla 👍

README.md Outdated Show resolved Hide resolved
@@ -977,6 +1001,20 @@ func checkPassthroughArg(target reflect.Value) bool {
}
}

func checkXorDuplicatedAndXandMissing(paths []*Path) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't exactly see why we'd like to join the errors from checkXorDuplicates() and checkXandMising() here. Is it in case there are variables that are included in both Xor and Xand groups and we have to combine error messages from missing Xors and Xands?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, I just thought it would be useful for the user to be told about both duplicated xor flag and missing xand flags in the same error message. So the user do not have to retry several times, and for example first get a message that there are two flags from the same xor group used, and then when fixing this get a new error message that there are missing xand flags and then needing to fix that. So from a user perspective I think it would be best to be told about both problems at the same time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I was slightly worried about breaking the single responsibility principle, but I think it makes sense in this case

Co-authored-by: Stautis <thkrst@gmail.com>
@tinytyranid tinytyranid merged commit 4a9bb74 into master Aug 6, 2024
@tinytyranid tinytyranid deleted the feature/xand branch August 6, 2024 08:36
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.

2 participants