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

decoder: Introduce helper functions for completion inside ObjectConsExpr #211

Merged
merged 3 commits into from
Feb 21, 2023

Conversation

radeksimko
Copy link
Member

@radeksimko radeksimko commented Feb 17, 2023

This PR introduces two small helper functions decoupled from a few discovered use cases / PRs:

The last attached test case illustrates an example of the problem.


Implementation Notes

Our use cases don't actually involve multi-byte runes, so in theory we could be just comparing single bytes and avoid the decoding, but that then makes the consumer side a bit more verbose since '\n' would need to be written e.g. as byte('\n'). We could technically allow runes and treat them like single bytes, but that just feels wrong.

Since we'd usually seek through relatively small slice, the decoding is unlikely to pose performance issue, but we can revisit the solution if it does.

@radeksimko radeksimko added the enhancement New feature or request label Feb 17, 2023
@radeksimko radeksimko marked this pull request as ready for review February 17, 2023 19:01
@radeksimko radeksimko requested a review from a team as a code owner February 17, 2023 19:01
@radeksimko radeksimko self-assigned this Feb 17, 2023
Copy link
Member

@dbanck dbanck left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small suggestion, feel free to merge anyway.

decoder/expression.go Outdated Show resolved Hide resolved
Co-authored-by: Daniel Banck <dbanck@users.noreply.github.com>
@radeksimko radeksimko merged commit 1454e5d into main Feb 21, 2023
@radeksimko radeksimko deleted the f-decoder-expr-helpers branch February 21, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants