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 export package-information command #4298

Merged
merged 14 commits into from
Mar 22, 2025

Conversation

Papipo
Copy link
Contributor

@Papipo Papipo commented Mar 4, 2025

Closes #4240

  • Adds Serialize trait to PackageConfig and all types it depends on.
  • Adds serde options to skip flags that have default values.
  • Adds gleam export package-information --out [out] command
  • Fixes gleam export package-interface format for gleam_version_constraint, which was being exported like: "gleam-version-constraint":"1.8.0 <= v" because it relied on pubgrub. Now it uses hexpm format and emits >= 1.8.0.

@Papipo Papipo marked this pull request as draft March 4, 2025 12:51
@Papipo Papipo force-pushed the add-export-package-info-command branch from 8db0fdd to e858ca1 Compare March 6, 2025 16:40
@Papipo Papipo marked this pull request as ready for review March 6, 2025 20:53
@Papipo Papipo force-pushed the add-export-package-info-command branch from d2d59b7 to 0d0f3df Compare March 9, 2025 18:08
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Wonderful, thank you!

Let's make it always render all the fields, even if they are the defaults. This will make it easier to understand what all the fields are.

It would also be good to have some round-trip tests, to ensure that a config file serialised and then deserialise doesn't have a bug that results in data-loss.

@lpil lpil marked this pull request as draft March 10, 2025 13:18
@Papipo Papipo force-pushed the add-export-package-info-command branch 3 times, most recently from c59d105 to f10e47a Compare March 15, 2025 13:21
@Papipo Papipo marked this pull request as ready for review March 15, 2025 13:37
@Papipo
Copy link
Contributor Author

Papipo commented Mar 15, 2025

@lpil I think all comments have been handled.

Let me know if I should squash some commits or something.

@Papipo Papipo changed the title Add export package info command Add export package-information command Mar 15, 2025
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you, but not all the comments have been resolved. Please remove all the serialisation skipping, the trait use, and merge the duplicate serde attribute.

@Papipo
Copy link
Contributor Author

Papipo commented Mar 16, 2025

Thank you, but not all the comments have been resolved. Please remove all the serialisation skipping, the trait use, and merge the duplicate serde attribute.

Dammit, I think I had some rebasing issue 😮‍💨

@lpil
Copy link
Member

lpil commented Mar 16, 2025

Ah I hate it when that happens!

@Papipo Papipo force-pushed the add-export-package-info-command branch from b90a873 to ad02832 Compare March 16, 2025 19:04
@Papipo
Copy link
Contributor Author

Papipo commented Mar 16, 2025

@lpil The duplicate serde attribute was removed rather than merged due to the removal of all skips.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Oh! The test failed due to ordering, but you've got preserve_order as a serde feature. Is that not enough to make the order deterministic or is there more to do there?

@Papipo
Copy link
Contributor Author

Papipo commented Mar 17, 2025

Oh! The test failed due to ordering, but you've got preserve_order as a serde feature. Is that not enough to make the order deterministic or is there more to do there?

I thought it was enough as you can expect, sigh. Gonna investigate as it!

@Papipo Papipo force-pushed the add-export-package-info-command branch from 4669f34 to 6f72a8b Compare March 17, 2025 15:15
@Papipo
Copy link
Contributor Author

Papipo commented Mar 17, 2025

Should be fixed now, it was my fault (obsolete snapshot).

@Papipo Papipo force-pushed the add-export-package-info-command branch from 6f72a8b to ce4c4b1 Compare March 18, 2025 12:07
@lpil
Copy link
Member

lpil commented Mar 18, 2025

Tests are failing!

@Papipo Papipo force-pushed the add-export-package-info-command branch from c52c70d to 403801c Compare March 18, 2025 22:30
Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you! Could you update the changelog please

Papipo added 3 commits March 19, 2025 13:29
gleam_version_constraint format was that of pubgrub's. Now it's hex's.
@Papipo Papipo force-pushed the add-export-package-info-command branch from e110ae1 to b8302ed Compare March 19, 2025 12:32
@Papipo
Copy link
Contributor Author

Papipo commented Mar 19, 2025

Thank you! Could you update the changelog please

Done and rebased due to conflicts.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Oh! I've just noticed that the top level {"gleam.toml": is missing!

@Papipo
Copy link
Contributor Author

Papipo commented Mar 19, 2025

Oh! I've just noticed that the top level {"gleam.toml": is missing!

Oh, nice catch. That was definitely lost when I trashed the first iteration. Working on it 🚀

@Papipo
Copy link
Contributor Author

Papipo commented Mar 19, 2025

In fact there is no test for this nesting thing, as the snapshots are for the PackageConfig serialisation and the nesting is happening at the export command level. Should I add a snapshop test for this?

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

We want the tests to test this output, not just a subset of that output 🙏 Move the nesting out of the command please and have the test do that new serialisation thingy

Ensure that JSON nesting is in the snapshot by introducing a new PackageInformation struct
@Papipo
Copy link
Contributor Author

Papipo commented Mar 20, 2025

We want the tests to test this output, not just a subset of that output 🙏 Move the nesting out of the command please and have the test do that new serialisation thingy

I introduced the PackageInformation struct to deal with these matters, let me know if it looks good now.

Copy link
Member

@lpil lpil left a comment

Choose a reason for hiding this comment

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

Thank you!!

@lpil lpil enabled auto-merge (rebase) March 22, 2025 10:58
@lpil lpil merged commit 5a770d6 into gleam-lang:main Mar 22, 2025
12 checks passed
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.

Add command to export deps/config
2 participants