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

Add a config field that controls package manifest generation #66

Merged
merged 17 commits into from
Mar 3, 2025

Conversation

temeddix
Copy link
Contributor

@temeddix temeddix commented Mar 1, 2025

Summary

Hi all,

First off, thanks for all the hard work you do maintaining this repository—it truly is a great work.

In this PR, I've updated the CodeGenerator.output method so that it ignores an existing pubspec.yaml, preventing it from being overwritten when using serde-generate for Dart code. I'm open to any modifications or suggestions, and I'll respond as quickly as possible to any feedback. This change is intended for use at Rinf.

Looking forward to your thoughts!

Test Plan

Empty

@temeddix temeddix requested a review from ma2bd as a code owner March 1, 2025 16:01
@temeddix
Copy link
Contributor Author

temeddix commented Mar 1, 2025

It's strange that the Golang test is failing when I’ve only made minor changes on the Dart side. Any suggestions, @ma2bd?

@temeddix temeddix changed the title Do not overwrite pubspec.yaml if it already exists Do not overwrite Dart's pubspec.yaml if it already exists Mar 1, 2025
@ma2bd
Copy link
Contributor

ma2bd commented Mar 1, 2025

Hey @temeddix, Thanks for the kind words. Rinf looks very interresting by the way. I'm going to look into the issue with golang in CI.

About this PR: If I'm reading the code correctly, this would make serde-generate not idempotent any more, which could be surprising to some users. Would you be ok adding a codegen option (say) --only-source-code to serde_generate_bin and CodeGeneratorConfig in order to explicitly skip the file in question? Bonus points for skipping similar metafiles in other languages.

@temeddix
Copy link
Contributor Author

temeddix commented Mar 2, 2025

Sure, I applied things you've mentioned. I discovered that I had to ask a few more questions:

  • Other languages were generating a package manifest in SourceInstaller.install_module. Is it okay for me to change the Dart system to match them?
    • Currently, it looks like Dart is the only language that generates a package manifest in CodeGenerator.output.
    • Perhaps Dart's package.yaml generation should go into SourceInstaller.intall_module too.
  • I declared the config field package_manifest: bool. Is this signature acceptable?

I'm open to any changes, so please let me know if anything needs improvement. I've already applied these changes to PR to demonstrate.

@temeddix temeddix changed the title Do not overwrite Dart's pubspec.yaml if it already exists Add a config field that controls package manifest generation Mar 2, 2025
/// Avoid creating a package spec file defining dependencies for the chosen language.
/// Takes effect only for languages that have a package manifest format.
#[structopt(long)]
no_package_manifest: bool,
Copy link
Contributor

@ma2bd ma2bd Mar 2, 2025

Choose a reason for hiding this comment

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

nit: This is fine but I'd suggest --skip-package-manifest here so that we can use the name skip_package_manifest consistently in the whole code, with a standard default value false.

Copy link
Contributor Author

@temeddix temeddix Mar 3, 2025

Choose a reason for hiding this comment

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

Done :) I realized that the default value for a clap bool field is false.

@ma2bd
Copy link
Contributor

ma2bd commented Mar 2, 2025

It's strange that the Golang test is failing when I’ve only made minor changes on the Dart side. Any suggestions, @ma2bd?

This should be fixed after #68

@ma2bd ma2bd merged commit 863be91 into zefchain:main Mar 3, 2025
2 checks passed
@ma2bd
Copy link
Contributor

ma2bd commented Mar 3, 2025

@temeddix Released in serde-generate v0.29.0 and serde-generate-bin v0.5.0

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.

2 participants