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

Add custom decider #51

Merged
merged 2 commits into from
May 31, 2024
Merged

Add custom decider #51

merged 2 commits into from
May 31, 2024

Conversation

alecsammon
Copy link
Contributor

An idea! Easier to demonstrate via a PR!

We use date based versioning instead of semantic versioning in our application. As such the default since/until don't work for us.

This PR adds the ability to overwrite the "decider" with a custom one. This allows any custom logic to be written to decide which fields should be included/excluded.

If a "decider" option is not provided then the existing logic will be used - as such this is a non-breaking change.

sheriff.go Outdated
@@ -10,8 +10,18 @@ import (
"github.com/hashicorp/go-version"
)

// A Decider is a function that decides whether a field should be marshalled or not.
// If it returns true, the field will be marshalled, otherwise it will be skipped.
type Decider func(field reflect.StructField) (bool, error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not convinced by the name! Very happy for recommendations here!

@mweibel
Copy link
Collaborator

mweibel commented May 31, 2024

Looks like a great addition! I'll think a bit about the name - it's not a bad name but I feel there might be a better option as well. Maybe something along the lines of IncludeProperty/Field but I don't think it's better.

@coveralls
Copy link

Coverage Status

coverage: 88.646% (+0.02%) from 88.626%
when pulling b8decc7 on alecsammon:custom_decider
into 270e679 on liip:master.

1 similar comment
@coveralls
Copy link

Coverage Status

coverage: 88.646% (+0.02%) from 88.626%
when pulling b8decc7 on alecsammon:custom_decider
into 270e679 on liip:master.

@coveralls
Copy link

coveralls commented May 31, 2024

Coverage Status

coverage: 88.646% (+0.02%) from 88.626%
when pulling 8dd1cf6 on alecsammon:custom_decider
into 270e679 on liip:master.

@mweibel
Copy link
Collaborator

mweibel commented May 31, 2024

ChatGPT suggested the following alternatives:

  • FieldSelector
  • FieldFilter
  • InclusionPredicate
  • FieldInclusionChecker
  • FieldEvaluator
  • FieldInclusionDecider
  • FieldCriteria
  • FieldValidator
  • IncludeField
  • ShouldIncludeField

Not sure. Thinking some more about it. Your opinion? 😄

@alecsammon
Copy link
Contributor Author

Ah - the hardest thing in programming! Naming things!

In your Readme you have this

Package sheriff marshals structs conditionally based on tags on the fields.

So maybe FieldCondition?
Or just Filter

@mweibel
Copy link
Collaborator

mweibel commented May 31, 2024

Indeed! 😄

Maybe FieldFilter could make sense? Since it filters fields after all. Conditionally yes, but I feel like it doesn't fit well as a function name somehow.

@alecsammon
Copy link
Contributor Author

Perfect: 8dd1cf6

@mweibel mweibel enabled auto-merge (rebase) May 31, 2024 06:40
@mweibel mweibel merged commit 60a4260 into liip:master May 31, 2024
4 checks passed
@alecsammon
Copy link
Contributor Author

Amazing - thank you!

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