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

Bd/fix path param ordering #100

Merged
merged 5 commits into from
Jul 26, 2019

Conversation

bavardage
Copy link
Contributor

Fixes #99

Possible downsides?

Fix isn't super pretty.

Part of me wonders whether this should actually end up being a conjure-ir spec change - whereby we assert in the IR the ordering matches up.

I'm also not super happy with the munging I have to do to make the special {thing:.+} style path params work. Such path parameters don't show up in the conjure docs (at least not in https://palantir.github.io/conjure/#/docs/spec/conjure_definitions?id=path-parameters) but are part of the test suite.

@bavardage bavardage requested a review from a team as a code owner July 24, 2019 03:33
@changelog-app
Copy link

changelog-app bot commented Jul 24, 2019

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Bd/fix path param ordering

Fixes #99

Possible downsides?

Fix isn't super pretty.

Part of me wonders whether this should actually end up being a conjure-ir spec change - whereby we assert in the IR the ordering matches up.

I'm also not super happy with the munging I have to do to make the special {thing:.+} style path params work. Such path parameters don't show up in the conjure docs (at least not in https://palantir.github.io/conjure/#/docs/spec/conjure_definitions?id=path-parameters) but are part of the test suite.

Check the box to generate changelog(s)

  • Generate changelog entry

@@ -222,3 +229,13 @@ function mediaType(conjureType?: IType) {
}
return "MediaType.APPLICATION_JSON";
}

function parsePathParamsFromPath(httpPath: string): string[] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so I could replace this method with

[... httpPath.matchAll(/\{([^:\}]+)([:]*[^\}]*)\}/g)].map(x => x[1])

but not convinced that's easier to read

@bavardage
Copy link
Contributor Author

@ferozco could you help fix the changelog? I don't have the ability to edit the bot's comment

@bulldozer-bot bulldozer-bot bot merged commit bae8d46 into palantir:develop Jul 26, 2019
@bavardage bavardage deleted the bd/fix-path-param-ordering branch July 26, 2019 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generated typescript incorrect if endpoint path args ordering doesn't match the order they appear in the path
3 participants