Report major breaking change type in JSON report and support pub refs without version #189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
devmil
requested review from
adamgic,
fwagner,
fabiomcarneiro and
klink182
as code owners
…t_major_in_json_for_breaking_changes
fabiomcarneiro
approved these changes
Oct 14, 2024
| test( | ||
| 'diffing cloud_firestore 4.3.1 to latest works', | ||
| () async { | ||
| // just some random package for testing the diff command for pub refs without version |
There was a problem hiding this comment.
I saw that we are repeating ourselves with this comment of 'just some random package ...'. What do you think about adding this comment at the top of the test file instead of repeating it for every test? I know that it will (probably) be harder to understand where the package came from but I don't think that repeating it for all the tests is much better too :/
Comment on lines
+83
to
+84
| }); | ||
| test('Can handle diff report with only one breaking change', () async { |
There was a problem hiding this comment.
Suggested change
| }); | |
| test('Can handle diff report with only one breaking change', () async { | |
| }); | |
| test('Can handle diff report with only one breaking change', () async { |
same for the other ones below if you agree with it
| addBreakingChange(diffResult, changeCode: ApiChangeCode.ci01); | ||
| await reporter.generateReport(diffResult, anyVersionCheckResult); | ||
| final jsonReport = jsonDecode(collectedFileContent.toString()); | ||
| expect(jsonReport['report']['breakingChanges'], isNotNull); |
There was a problem hiding this comment.
Suggested change
| expect(jsonReport['report']['breakingChanges'], isNotNull); | |
| expect(jsonReport['report']['breakingChanges'], isNotNull); |
I would create a method to return the breaking changes property and another one to get the 'children' of it. Otherwise, if we need to change the name of any of it, we would need to adapt a lot of tests
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR fixes the JSON report to specify "major" breaking changes in the type field in the JSON report and it adds support for omitting the version in pub refs. In that case dart_apitool will install the package without version constraints using pub and afterwards just use the latest version available in the cache.
Type of Change
Fixes #188
Fixes #186