Skip to content

matchFieldName and matchFieldOptionValue options#30

Merged
gr2m merged 8 commits intomainfrom
25-add-matchfieldname-and-matchfieldoptionvalue-options-for-custom-matching-of-field-names-and-field-option-values
Mar 15, 2022
Merged

matchFieldName and matchFieldOptionValue options#30
gr2m merged 8 commits intomainfrom
25-add-matchfieldname-and-matchfieldoptionvalue-options-for-custom-matching-of-field-names-and-field-option-values

Conversation

@gr2m
Copy link
Copy Markdown
Owner

@gr2m gr2m commented Mar 12, 2022

@mikesurowiec
Copy link
Copy Markdown
Collaborator

Looks great!

@gr2m gr2m marked this pull request as ready for review March 15, 2022 00:01
Copy link
Copy Markdown
Owner Author

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Now that we offer custom matching functions, there is a chance that multiple fields / field options can be matched, which I'd say should throw a helpful error. But we can address this in a follow up issue, I'd say it's an edge case and shouldn't block this feature from being released

@gr2m gr2m requested a review from mikesurowiec March 15, 2022 00:06
Copy link
Copy Markdown
Collaborator

@mikesurowiec mikesurowiec left a comment

Choose a reason for hiding this comment

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

👏 awesome stuff!

Comment thread README.md
1. `projectFieldName`
2. `userFieldName`

Both are strings. Both arguments are lower-cased and trimmed before passed to the function. The function must return `true` or `false`.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is because GitHub lower-case + trims the field names, right? As in the user can't possibly attempt a case-sensitive match

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Yes, exactly

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Oh which made me think to check if option values are trimmed by GitHub, and indeed they are, I'll adjust the code and then merge it

@gr2m gr2m merged commit 0666b9a into main Mar 15, 2022
@gr2m gr2m deleted the 25-add-matchfieldname-and-matchfieldoptionvalue-options-for-custom-matching-of-field-names-and-field-option-values branch March 15, 2022 06:04
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.

add matchFieldName and matchFieldOptionValue options for custom matching of field names and field option values

2 participants