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: trailing comma not allowed in ruleset #634

Merged
merged 1 commit into from
Jan 23, 2023

Conversation

eduardomourar
Copy link
Contributor

Issue #, if available:
#633

Description of changes:

This will generate a valid TypeScript file for src/endpoint/ruleset.ts.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@eduardomourar eduardomourar requested a review from a team as a code owner November 10, 2022 10:56
@eduardomourar eduardomourar changed the title fix: trailing command not allowed in ruleset fix: trailing comma not allowed in ruleset Nov 10, 2022
*Issue #, if available:*
smithy-lang#633

*Description of changes:*

This will generate a valid TypeScript file for
`src/endpoint/ruleset.ts`.
@eduardomourar
Copy link
Contributor Author

@jvschneid , could this be approved and merged, please?

Copy link
Contributor

@gosar gosar left a comment

Choose a reason for hiding this comment

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

I added this comment to the issue. But looking at the code and your diff here, I can see that it would generate , and is likely a prettify step that's fixing it in AWS SDK. To showcase that more directly, and for adding some missing testing here, can we add a test that would be broken before your change and fixed with this.

@gosar gosar merged commit 068f4f7 into smithy-lang:main Jan 23, 2023
@eduardomourar eduardomourar deleted the fix/trailing-comma-ruleset branch January 24, 2023 00:52
@eduardomourar
Copy link
Contributor Author

@gosar, create PR #674 to include a test case called containsTrailingSemicolon where now it has been fixed.

@gosar
Copy link
Contributor

gosar commented Jan 24, 2023

@gosar, create PR #674 to include a test case called containsTrailingSemicolon where now it has been fixed.

Thanks @eduardomourar! I merged your PR as I didn't want to hold up the fix and was going to work on adding tests later. Thanks for adding them :)

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