-
Notifications
You must be signed in to change notification settings - Fork 76
Added the toJson method in Pubspec
#2214
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
base: main
Are you sure you want to change the base?
Added the toJson method in Pubspec
#2214
Conversation
Closes dart-lang#1801 - Added the serializable to serialize object into . - Added tests to validate functionality round-trip the shape of the source. - Updated to document the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully adds the toJson method to Pubspec and its related classes, enabling serialization. The implementation is correct and is well-covered by new round-trip tests, which is excellent. My review includes a few suggestions, mainly about adding documentation to new public members to adhere to the repository's style guide.
I also noticed that this pull request includes a significant refactoring of tests in the watcher package. While these changes seem to improve test stability, it's generally better to keep unrelated changes in separate pull requests. This helps keep the commit history clean and makes reviews more focused. For this review, I have focused only on the changes related to pubspec_parse.
lrhn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style comments only. I have no opinion on whether this is a good addition.
- Make serializeDeps private (_serializeDeps) - Use concise literals (for-in, null-aware spread) in screenshots, executables, and deps - Clarify _fromJson docs and tidy formatting across dependency/pubspec files - Regenerate pubspec.g.dart for updated serializer and formatting
lrhn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style seems fine.
Someone who actually knows what this code does should review the actual change.
We'll probably want someone from @dart-lang/dart-pub-team to confirm they are happy with the API. |
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
PR HealthLicense Headers ✔️
All source files should start with a license header. Unrelated files missing license headers
This check can be disabled by tagging the PR with API leaks ✔️The following packages contain symbols visible in the public API, but not exported by the library. Export these symbols or remove them from your publicly visible API.
This check can be disabled by tagging the PR with Breaking changes ✔️
This check can be disabled by tagging the PR with Coverage ✔️
This check for test coverage is informational (issues shown here will not fail the PR). This check can be disabled by tagging the PR with Changelog Entry ✔️
Changes to files need to be accounted for in their respective changelogs. This check can be disabled by tagging the PR with |
sigurdm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
@Dhruv-Maradiya any plans to land this? That would be amazing! |
|
Hi @mosuem I just resolved all comments and pushed the code, should I implement |
|
They do sound reasonable! As soon as those are addressed and the CI is green we can merge it. Thanks! |
…d Screenshot classes
Description
Fixes #1801 by adding
toJsonmethod inPubspecclass.Changes
toJsonserializable to serialize object intomap.CHANGELOG.mdto document the changes.Checklist