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

Remove requirement of -go-package from typeshare with go feature #184

Merged
merged 6 commits into from
Aug 28, 2024

Conversation

MOmarMiraj
Copy link
Contributor

@MOmarMiraj MOmarMiraj commented Jul 17, 2024

Currently the typeshare CLI with go feature enabled, doesn't allow the usage of package in typeshare.toml as the CLI requires the --go-package to be there. This MR will tackle that issue by not requiring the --go-package.

I also added a check to ensure that the package name is passed as this is a requirement in Golang.

I also added a UT for reading the package name from a typeshare.toml

@MOmarMiraj MOmarMiraj force-pushed the omar/fix-go-package-flag branch 2 times, most recently from 7987345 to e390aea Compare July 31, 2024 18:59
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@seanaye seanaye left a comment

Choose a reason for hiding this comment

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

2 small suggestions, otherwise LGTM, can approve after

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
Add check to see if no package flag is passed or in typeshare.toml

Fmt fix

clippy fix

Cleanup logic and make it cleaner

fix logic

fix return value

cargo clippy fix

Cleanup function

fix logic of go_package function

update logic for error handling

fix logic to correctly return package name

Capitalize all variant names when configuration specifies so in Go

Signed-off-by: Omar Miraj <[email protected]>

Add support for Kotlin inline value classes

Signed-off-by: Omar Miraj <[email protected]>

Introduce redacted typeshare parameter

Signed-off-by: Omar Miraj <[email protected]>

keep the file output deterministic

Signed-off-by: Omar Miraj <[email protected]>

Deterministic output

I want to use a parallel iterator while walking the input folders however in order to maintain a deterministic output I'll have to keep the crate to parsed data sorted along with the types within each parsed data. Consequently the tests have been updated since they did not have this sorting before.

Signed-off-by: Omar Miraj <[email protected]>

Add determinism ensurance tests

Signed-off-by: Omar Miraj <[email protected]>

chore: update changelog and bump version numbers for release v1.10.0-beta.7

Signed-off-by: Omar Miraj <[email protected]>

Explicitly format variant types in Go

Signed-off-by: Omar Miraj <[email protected]>

Remove dead code and switch if else to match

Signed-off-by: Omar Miraj <[email protected]>

Add check to see if no package flag is passed or in typeshare.toml

Fmt fix

Signed-off-by: Omar Miraj <[email protected]>

Cleanup logic and make it cleaner

update logic for error handling

fix logic to correctly return package name

Capitalize all variant names when configuration specifies so in Go

Signed-off-by: Omar Miraj <[email protected]>

keep the file output deterministic

Deterministic output

I want to use a parallel iterator while walking the input folders however in order to maintain a deterministic output I'll have to keep the crate to parsed data sorted along with the types within each parsed data. Consequently the tests have been updated since they did not have this sorting before.

Signed-off-by: Omar Miraj <[email protected]>

Explicitly format variant types in Go

Signed-off-by: Omar Miraj <[email protected]>

Remove dead code and switch if else to match

Signed-off-by: Omar Miraj <[email protected]>
Copy link
Collaborator

@seanaye seanaye left a comment

Choose a reason for hiding this comment

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

Sorry to re-request changes but I think we could make this a little more idiomatic (and also avoid and extra string allocation)

cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Outdated Show resolved Hide resolved
cli/src/main.rs Show resolved Hide resolved
@seanaye seanaye merged commit a905fc1 into 1Password:main Aug 28, 2024
6 checks passed
@MOmarMiraj MOmarMiraj deleted the omar/fix-go-package-flag branch August 28, 2024 15:50
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.

None yet

3 participants