Skip to content

suggestions #4219

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

Conversation

yaacovCR
Copy link
Contributor

@yaacovCR yaacovCR commented Oct 7, 2024

includes:

  1. changes just to reduce the diff
  2. use of Maybe for some of our exported helper functions to match rest of our API
  3. reverts some of my previous suggestions revolving around collectFields() => i was under the mistaken assumption that we never needed to pass maskSuggestions to collectFields, but because of fragment arguments, we might have complex inputs and we might need to mask suggestions
  4. errata

@yaacovCR yaacovCR requested a review from a team as a code owner October 7, 2024 19:45
@yaacovCR yaacovCR requested review from JoviDeCroock and removed request for a team October 7, 2024 19:45
Copy link

github-actions bot commented Oct 7, 2024

Hi @yaacovCR, I'm @github-actions bot happy to help you with this PR 👋

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

@yaacovCR
Copy link
Contributor Author

yaacovCR commented Oct 7, 2024

I'm not totally sure about the argument ordering, I'm not happy to separate variableValues and fragmentVariableValues, and I'm not happy to put maskSuggestions earlier, because that would be a breaking change, so I settled on putting maskSuggestions later, which is fine, except that if we ever revert the fragment arguments experiments, that would end up being a breaking change... Now, I don't ==think== we are going to revert it.... thoughts on that minor issue?

Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

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

I'm sorry but in general this feels very nitpicky about argument ordering 😅 all for avoiding a breaking change and maybe it's better to even use options consistently in collectFields as well but all the other changes are styllistic

@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 2 times, most recently from 44b356c to ff2087d Compare October 8, 2024 07:12
@yaacovCR yaacovCR force-pushed the suggestion-for-masking-suggestions branch from 4a1ef07 to 268f8fc Compare October 8, 2024 07:22
@JoviDeCroock JoviDeCroock force-pushed the allow-skipping-suggestions branch 3 times, most recently from d6af6ea to 10475bb Compare October 8, 2024 07:32
@yaacovCR yaacovCR closed this Oct 8, 2024
@yaacovCR yaacovCR deleted the suggestion-for-masking-suggestions branch October 8, 2024 07:40
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