-
Notifications
You must be signed in to change notification settings - Fork 114
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 a way to say "all fields" in the projection #155
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,7 +178,12 @@ func (p *projectionParser) scanFieldName() string { | |
field := []byte{} | ||
for p.more() { | ||
c := p.peek() | ||
if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' { | ||
if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || (c >= '0' && c <= '9') || c == '_' || c == '-' || c == '*' { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, will fix this. |
||
// Allow * only by itself. | ||
if (c == '*' && len(field) != 0) || (c != '*' && len(field) > 1 && field[0] == '*') { | ||
return "" | ||
} | ||
|
||
field = append(field, c) | ||
p.pos++ | ||
continue | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 for loop is a bit awkward...
Could not this whole test be just:
This may give a slightly more clear LOS (Line of Sight).
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.
Otherwise I am now OK with
*
only being allowed as the only parameter, as long as this now expands all references; at least initially. May place nice (end-user-wise) with the suggesrion of using*
to expand references in schema.Dict and schema.Array#138
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.
Thanks for the suggestions @smyrman. However expanding referenced fields, is what I wanted to avoid in the first place. My indention was to provide extension to this requests:
Which will return only non referenced fields, so making this request:
Is to actually add the
visits
referenced resource, without mentioning all the non referenced fields.Initially I though this as a low hanging fruit, but it gets more complicated. It is not big deal to enumerate all the fields explicitly, so I think I will just abandon this PR.
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.
Alright 👍 Might be wise for now. This is also the way we use it.
Just for my clarification though.
This is also what I though was the initial intent. However, my (perhaps wrongful) understanding of the final PR was that it would expand "visits", all other references and maybe (not clear to me) all fields that are hidden by default (Field.Hidden == true) if you did this:
If we rather want the initially described behaviour, I suppose the following might work:
And then no other changes. This will also ensure errors later if
*
is part of a field-name to be as before without the extra checks added in this PR, and withourt accepting*
in the legal field character list.Anyway, still maybe best to leave it for now, and consider it a bit closer.
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.
The request above would have been equivalent to this:
No referenced fields, no hidden fields.
Actually there is no way of getting the hidden fields with the current code. Mentioning them explicitly in the
fields=
parameter results in the following error: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.
Closing the PR now.
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.
Thanks for your effort, even if it was not merged this time.