-
Notifications
You must be signed in to change notification settings - Fork 28
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
Generating a changelog for a monorepository #1090
base: epic/ck/18051-changeset
Are you sure you want to change the base?
Generating a changelog for a monorepository #1090
Conversation
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.
First review part.
packages/ckeditor5-dev-changelog/src/utils/providenewversionformonorepository.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-dev-changelog/src/utils/providenewversionformonorepository.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-dev-changelog/src/utils/providenewversionformonorepository.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-dev-changelog/src/utils/providenewversionformonorepository.ts
Outdated
Show resolved
Hide resolved
Please merge |
Is the lack of |
It seems that current implementation expects that at least one major and at least one minor breaking change is found. import { generateChangelog } from '@ckeditor/ckeditor5-dev-changelog';
generateChangelog( {
cwd: process.cwd()
} ); throws an error:
|
packages/ckeditor5-dev-changelog/src/utils/providenewversionformonorepository.ts
Outdated
Show resolved
Hide resolved
packages/ckeditor5-dev-changelog/src/utils/logchangelogfiles.ts
Outdated
Show resolved
Hide resolved
fd4c5a1
to
8ed77ce
Compare
483a500
to
0a47dab
Compare
0a47dab
to
e787fa7
Compare
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.
Found a few things to consider. Other than that, LGTM.
-
Blank line after the version question?
○ Determining the new version... ✔ Type the new version (current: "44.3.0", suggested: "45.0.0"): 45.0.0 ○ Appending changes to the existing changelog... ○ Removing the changeset files...
-
The generated changelog contains a space after the entry, but the source changeset file does not have it:
"@ckeditor/ckeditor5-dev-release-tools": "^47.0.0", | ||
"chalk": "^5.0.0", | ||
"date-fns": "^4.0.0", | ||
"pacote": "^19.0.0", | ||
"fs-extra": "^11.0.0", | ||
"glob": "^10.0.0", | ||
"gray-matter": "^4.0.3", | ||
"inquirer": "^11.0.0", | ||
"semver": "^7.6.3", | ||
"upath": "^2.0.1" |
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.
Please sort dependencies alphabetically.
cwd, | ||
packagesDirectory = PACKAGES_DIRECTORY_NAME, | ||
organisationNamespace = ORGANISATION_NAMESPACE, | ||
nextVersion, | ||
externalRepositories = [], | ||
transformScope = defaultTransformScope, | ||
date = format( new Date(), 'yyyy-MM-dd' ) as RawDateString, | ||
changesetsDirectory = CHANGESET_DIRECTORY |
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.
Two proposals:
- The
cwd
option could also have default value, so you don't have to specifyprocess.cwd()
in 5 other places. - Options with default values could be listed at the end to quickly find out what is required and what may be skipped.
{ | ||
changesetPaths: await glob( '**/*.md', { cwd: upath.join( cwd, changesetsDirectory ), absolute: true } ), | ||
gitHubUrl: await getRepositoryUrl( cwd ), | ||
skipLinks: false |
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.
What if we would like to skip links in the generated changelog for the root repository, e.g. when the repository is private?
* Applies default values to the external repositories configuration. | ||
*/ | ||
export function getExternalRepositoriesWithDefaults( externalRepositories: Array<RepositoryConfig> ): Array<Required<RepositoryConfig>> { | ||
return externalRepositories.map( repo => ( { |
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.
This line is not consistent with others places:
externalRepositories.map( externalRepository => … )
externalRepositories.map( repo => … )
/** | ||
* This function locates package.json files for all packages located in `packagesDirectory` in the repository structure. | ||
*/ | ||
export async function findPathsToPackages( cwd: string, packagesDirectory: string | null, options: Options = {} ): Promise<Array<string>> { |
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.
The findPathsToPackages()
function is always called with includePackageJson = true
, so maybe this option could be removed.
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.
A few utils are not mocked: getDateFormatted
, defaultTransformScope
, getExternalRepositoriesWithDefaults
.
}; | ||
|
||
beforeEach( () => { | ||
vi.clearAllMocks(); |
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.
It seems that vi.clearAllMocks()
is not needed, as we have test.restoreMocks = true
in vitest.config.ts
.
|
||
describe( 'getChangesetFilePaths', () => { | ||
beforeEach( () => { | ||
vi.mocked( upath.join ).mockImplementation( ( ...paths ) => paths.join( '/' ) ); |
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.
From my point of view it is not needed to mock upath
. Let's treat this as an external library, like lodash
.
} | ||
} else { | ||
for ( const entry of section.entries ) { | ||
logInfo( `- "${ entry.data.mainContent }" (file://${ entry.changesetPath })`, { indent: 4 } ); |
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.
It produces paths with backslashes on Windows which are not clickable:
◌ Found Invalid changes:
- "Message." (file://D:\Projects\ckeditor\ckeditor5\.changelog\1.md)
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.
This proposal #1090 (comment) should fix it.
...externalChangesetPaths | ||
] ); | ||
|
||
return resolvedChangesetPaths; |
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.
It would be nice to normalize all paths. On Windows glob
returns paths with backslash separators.
Suggested merge commit message (convention)
Feature: Generating a changelog for a monorepository. Closes ckeditor/ckeditor5#18063.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.