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 validate command #28

Merged
merged 6 commits into from
Apr 30, 2021
Merged

Add validate command #28

merged 6 commits into from
Apr 30, 2021

Conversation

Gudahtt
Copy link
Member

@Gudahtt Gudahtt commented Apr 20, 2021

The validate command will validate the formatting of the changelog. If the --rc flag is used, it will also ensure that the current version is included as a release header, and that there are no unreleased changes.

The CLI command displays a rudimentary diff if formatting problems are detected. This can be improved later with colours, and with highlighting within each line to show what has changed. It also doesn't yet highlight whitespace changes.

The validation logic is also exposed via the API as a function called validateChangelog. It doesn't include the fancy diff output, but it does throw an error with all of the required information for someone to construct the same output.

Closes #11

@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 20, 2021

This depends upon #26 and #29

@Gudahtt Gudahtt force-pushed the add-validate-command branch 3 times, most recently from c826108 to cf1da06 Compare April 20, 2021 18:32
Base automatically changed from move-update-to-subcommand to main April 20, 2021 19:41
@Gudahtt Gudahtt changed the base branch from main to fix-release-header-sorting April 20, 2021 19:42
Base automatically changed from fix-release-header-sorting to main April 20, 2021 21:09
@Gudahtt Gudahtt marked this pull request as ready for review April 20, 2021 21:09
@Gudahtt Gudahtt requested a review from a team as a code owner April 20, 2021 21:09
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 21, 2021

Rebased to resolve conflicts

configureCommonCommandOptions(_yargs)
.option('rc', {
default: false,
description: `Verify that the current version has a release header in the changelog`,
Copy link
Member Author

Choose a reason for hiding this comment

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

I left rc out of the "commonCommandOptions" group because it was helpful to have a separate description for this flag for each command, since it implies something a bit different in each case.

@Gudahtt Gudahtt force-pushed the add-validate-command branch 2 times, most recently from 592aedd to 57f1a08 Compare April 21, 2021 15:04
src/validateChangelog.js Outdated Show resolved Hide resolved
The validate command will validate the formatting of the changelog.
If the `--rc` flag is used, it will also ensure that the current
version is included as a release header, and that there are no
unreleased changes.

The CLI command displays a rudimentary diff if formatting problems are
detected. This can be improved later with colours, and with
highlighting within each line to show what has changed. It also doesn't
yet highlight whitespace changes.

The validation logic is also exposed via the API as a function called
`validateChangelog`. It doesn't include the fancy diff output, but it
does throw an error with all of the required information for someone to
construct the same output.

Tests have been added to comprehensively test the changelog validation.

Closes #11
All invalid changelog cases are now described by the
`InvalidChangelogError` error type, which has sub-classes to describe
each possible scenario. This allows distinguishing invalid changelog
errors from all other unexpected errors.

The CLI has been updated to take advantage of this; we now print a
simpler error message if the changelog is invalid, rather than a stack
trace.
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

This was great to review. Well conceived structure, great tests, and neat use of diffing!

I don't have any blocking outright change requests, but rather questions and comments that should be addressed before merging.

src/generateDiff.js Outdated Show resolved Hide resolved
src/generateDiff.js Outdated Show resolved Hide resolved
src/generateDiff.js Outdated Show resolved Hide resolved
src/validateChangelog.test.js Outdated Show resolved Hide resolved
src/cli.js Show resolved Hide resolved
src/cli.js Outdated Show resolved Hide resolved
The `generateDiff` function now has a comprehensive test suite. It has
been updated to ensure that the resulting diff displays a notice about
whether or not the file has a trailing newline.

I started this journey by trying to ensure we were trimming each line
properly. I found that `diff.diffLines` always returned change sets
that ended in a newline, except when returning the very last change to
a file that didn't terminate in a newline. So in order to ensure we
were correctly showing all changes, I needed to add special handling
for end-of-file changes. We could display this differently than I've
chosen to here, but I figured showing what `git` shows was a good
start.

The `generateDiff` function now also ensures the correct context is
shown in between each change.

The dependency `outdent` was added to help with writing inline test
fixtures. This saves us from having to come up with a name for each
fixture and cross-reference them in each test.

When adding that dependency, an ESLint error brought to my attention
that we were accidentally including test files in our `files` property
in `package.json`. It was updated to exclude test files, which resolved
the ESLint error.
The passing test cases have been grouped together at the beginning of
the main `describe` block.
Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

Alright, one last round!

src/generateDiff.js Outdated Show resolved Hide resolved
src/generateDiff.js Outdated Show resolved Hide resolved
src/generateDiff.js Outdated Show resolved Hide resolved
src/generateDiff.test.js Show resolved Hide resolved
src/generateDiff.js Show resolved Hide resolved
@Gudahtt
Copy link
Member Author

Gudahtt commented Apr 28, 2021

Just pushed one more commit to improve variable names and comments. I think everything has been addressed - let me know if not!

Copy link
Member

@rekmarks rekmarks left a comment

Choose a reason for hiding this comment

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

LGTM!

@Gudahtt Gudahtt merged commit b1fbbbe into main Apr 30, 2021
@Gudahtt Gudahtt deleted the add-validate-command branch April 30, 2021 19:44
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.

Expose API method and CLI command for validating changelog
2 participants