-
Notifications
You must be signed in to change notification settings - Fork 5
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 tokenStart and tokenEnd to query #9
Conversation
I also would like to make the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good feature to add, but please:
- add tests, the more the better to see (a) what is now available and (b) how to use/access it (it can be a new file)
- explain a bit how the optional
exp
is breaking, and maybe also add tests there too with comments - also update the changelog and bump the version
Sure. I didn't want to write them until you gave it an ok :P
It was failing one test but I was able to make a minor change
Cool. Will do after the tests |
… keep it backwards compatible
I've changed |
I will look the code later, but if this is a change that could possibly break more than one API, maybe it is worth doing it by bumping the major version and documenting what has been changed in a breaking way. Not yet sure if it is the case here, I'm just stating that it is not off the table. |
If you're happy with a major version bump then that'd make things easier. |
I don't mind, just make sure to add plenty of notes in the changelog to help users migrate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a high level I like this, but I'd like to do a deep dive into it before going forward with it. Unfortunately I have a few busy days before me, and it may happen only sometime next week :(
No prob. I'm using this branch in my project for now so no rush |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all the changes you've done! Only one big question remains about the type of the operator
.
For the other two, I'm happy to do them, but I'm unsure which name to use for the span field/class. Any preference?
final String operator; | ||
class FieldCompareQuery extends Query { | ||
final TextQuery field; | ||
final TextQuery operator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the change from String
to TextQuery
for the operator
field correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could help figure out where the operator is.
e.g. all of these are valid:
field=value
field = value
field = value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be a separate FieldOperator extends Query
type, with text
or value
+ position
. The TextQuery
has isExactMatch
which doesn't make sense for this field, and it doesn't feel right. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it make sense to have FieldCompareQuery.field
, FieldCompareQuery .operator
and FieldScope.operator
all share the same class. e.g. something like
class FieldElementQuery extends Query {
final String text;
const FieldElementQuery({
required this.text,
required super.position,
});
@override
String toString({bool debug = false}) =>
_debug(debug, text);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or... Remove isExactMatch
from TextQuery as its only ever used for PhraseQuery
?
I think this is what I prefer but I'm happy to for whatever you suggest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good solution too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! One small nit for the documentation part.
…ry` is always exact and `TextQuery` never is.
I'm really happy with the current state of the PR, thank you for idea and all the small follow-ups! |
Great features! :) Published as 2.0.0 |
I wanted to be able to offer auto complete suggestions for my search query and for that I needed to know the location of the
Query
to figure out if it was at my cursor position.This PR adds the start / end position of the
Query
relative toinput
.