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

fix: indent and commas #101

Merged
merged 8 commits into from
Nov 18, 2021
Merged

fix: indent and commas #101

merged 8 commits into from
Nov 18, 2021

Conversation

o0Ignition0o
Copy link
Contributor

While formatting .graphql files, my linter added commas and newlines around. Although this is allowed in the spec, and the parser handles it correctly, we weren't able to handle it when gathering endpoint names and urls.

This commits fixes this, and adds various indents and commas in the routing_urls test.

While formatting .graphql files, my linter added commas and newlines around. Although this is allowed in the spec, and the parser handles it correctly, we weren't able to handle it when gathering endpoint names and urls.

This commits fixes this, and adds various indents and commas in the routing_urls test.
@o0Ignition0o o0Ignition0o requested review from lrlna and Geal November 8, 2021 20:18
@o0Ignition0o o0Ignition0o marked this pull request as draft November 9, 2021 08:03
@o0Ignition0o o0Ignition0o marked this pull request as ready for review November 9, 2021 11:06
@o0Ignition0o
Copy link
Contributor Author

marking it back as draft, until apollo-rs provides us with a nice api that avoids the temporary workaround :)

@o0Ignition0o o0Ignition0o marked this pull request as draft November 9, 2021 15:24
Comment on lines 118 to 135
let arg_value =
argument.value().and_then(|s| {
s.to_string()
.trim_end()
.strip_prefix('"')
.and_then(|s| s.strip_suffix('"'))
.map(|s| s.to_owned())
// This is a temporary workaround until we have nice semantic analysis
s.syntax()
.green()
.children()
.next()
.and_then(|it| it.into_token())
.map(|token| {
token
.text()
.trim()
.trim_start_matches('"')
.trim_end_matches('"')
.trim()
.to_string()
})
});
Copy link
Member

Choose a reason for hiding this comment

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

if you update to the latest apollo-rs commit you can get the STRING_VALUE as rust's String type:

if let ast::Value::StringValue(val) =
    argument.value().expect("Cannot get argument value.")
{
    let s: String = val.into();
}

It should help you eliminate this green node manipulation happening here.

Copy link
Member

Choose a reason for hiding this comment

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

^ It is still preferable that you match on the variants of argument.value(), as not all variants implement casting to native rust types (they are not likely to in the future either).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it works! 🙌 thank you! 🚀

@o0Ignition0o o0Ignition0o self-assigned this Nov 17, 2021
@o0Ignition0o o0Ignition0o marked this pull request as ready for review November 17, 2021 17:50
@o0Ignition0o o0Ignition0o requested review from cecton and lrlna November 17, 2021 17:50
Copy link
Member

@lrlna lrlna left a comment

Choose a reason for hiding this comment

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

ahhh very nice! i like the way this looks.

Copy link
Contributor

@cecton cecton left a comment

Choose a reason for hiding this comment

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

LGTM though I don't have opinion on styling XD

@o0Ignition0o o0Ignition0o merged commit b075c74 into main Nov 18, 2021
@o0Ignition0o o0Ignition0o deleted the igni/fix_indent_and_commas branch November 18, 2021 10:17
@abernix abernix mentioned this pull request Nov 19, 2021
tinnou pushed a commit to Netflix-Skunkworks/router that referenced this pull request Oct 16, 2023
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.

3 participants