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.

Sign up for GitHub

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

Jump to bottom

Get package version from manifest #121

Merged
merged 3 commits into from
Nov 4, 2022
Merged

Get package version from manifest #121

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
aaf504e
Get package version from manifest
Gudahtt Nov 2, 2022
41cb1e6
Re-throw unhandled error
Gudahtt Nov 3, 2022
d052c76
Merge branch 'main' into get-package-version-from-manifest
Gudahtt Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 51 additions & 24 deletions src/cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ const validateEpilog = `This does not ensure that the changelog is complete, \
or that each change is in the correct section. It just ensures that the \
formatting is correct. Verification of the contents is left for manual review.`;

// eslint-disable-next-line node/no-process-env
const npmPackageVersion = process.env.npm_package_version;

/**
* Determine whether the given URL is valid.
*
Expand Down Expand Up @@ -256,7 +253,6 @@ async function main() {
type: 'boolean',
})
.option('currentVersion', {
default: npmPackageVersion,
description:
'The current version of the project that the changelog belongs to.',
type: 'string',
Expand All @@ -274,7 +270,6 @@ async function main() {
type: 'boolean',
})
.option('currentVersion', {
default: npmPackageVersion,
description:
'The current version of the project that the changelog belongs to.',
type: 'string',
Expand All @@ -292,31 +287,13 @@ async function main() {
);

const {
currentVersion,
file: changelogFilename,
rc: isReleaseCandidate,
repo: repoUrl,
root: projectRootDirectory,
tagPrefix,
} = argv;

if (isReleaseCandidate && !currentVersion) {
exitWithError(
`Version not found. Please set the --currentVersion flag, or run this as an npm script from a project with the 'version' field set.`,
);
return;
} else if (currentVersion && semver.valid(currentVersion) === null) {
exitWithError(`Current version is not valid SemVer: '${currentVersion}'`);
return;
} else if (!repoUrl) {
exitWithError(
`npm package repository URL not found. Please set the '--repo' flag, or run this as an npm script from a project with the 'repository' field set.`,
);
return;
} else if (!isValidUrl(repoUrl)) {
exitWithError(`Invalid repo URL: '${repoUrl}'`);
return;
}
let { currentVersion } = argv;

if (projectRootDirectory) {
try {
Expand All @@ -343,6 +320,56 @@ async function main() {
}
}

if (!currentVersion) {
const manifestPath = projectRootDirectory
? path.join(projectRootDirectory, 'package.json')
: path.resolve('package.json');

try {
const manifestText = await fs.readFile(manifestPath, {
encoding: 'utf-8',
});
const manifest = JSON.parse(manifestText);
currentVersion = manifest.version;
} catch (error) {
if (error.code === 'ENOENT') {
exitWithError(
`Package manifest not found at path: '${manifestPath}'\nRun this script from the project root directory, or set the project directory using the '--root' flag.`,
);
return;
} else if (error.code === 'EACCES') {
exitWithError(
`Access to package manifest is forbidden by file access permissions: '${manifestPath}'`,
);
return;
} else if (error.name === 'SyntaxError') {
exitWithError(
`Package manifest cannot be parsed as JSON: '${manifestPath}'`,
);
return;
}
Comment on lines +335 to +350
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (error.code === 'ENOENT') {
exitWithError(
`Package manifest not found at path: '${manifestPath}'\nRun this script from the project root directory, or set the project directory using the '--root' flag.`,
);
return;
} else if (error.code === 'EACCES') {
exitWithError(
`Access to package manifest is forbidden by file access permissions: '${manifestPath}'`,
);
return;
} else if (error.name === 'SyntaxError') {
exitWithError(
`Package manifest cannot be parsed as JSON: '${manifestPath}'`,
);
return;
}
if (error.code === 'ENOENT') {
return exitWithError(
`Package manifest not found at path: '${manifestPath}'\nRun this script from the project root directory, or set the project directory using the '--root' flag.`,
);
}
if (error.code === 'EACCES') {
return exitWithError(
`Access to package manifest is forbidden by file access permissions: '${manifestPath}'`,
);
}
if (error.name === 'SyntaxError') {
return exitWithError(
`Package manifest cannot be parsed as JSON: '${manifestPath}'`,
);
}
throw err;

throw error;
}
}

if (!currentVersion) {
exitWithError(
`Version not found. Please set the --currentVersion flag, or run this as an npm script from a project with the 'version' field set.`,
);
return;
} else if (currentVersion && semver.valid(currentVersion) === null) {
exitWithError(`Current version is not valid SemVer: '${currentVersion}'`);
return;
} else if (!repoUrl) {
exitWithError(
`npm package repository URL not found. Please set the '--repo' flag, or run this as an npm script from a project with the 'repository' field set.`,
);
return;
} else if (!isValidUrl(repoUrl)) {
exitWithError(`Invalid repo URL: '${repoUrl}'`);
return;
}
Comment on lines +356 to +371
Copy link
Contributor

@legobeat legobeat Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
exitWithError(
`Version not found. Please set the --currentVersion flag, or run this as an npm script from a project with the 'version' field set.`,
);
return;
} else if (currentVersion && semver.valid(currentVersion) === null) {
exitWithError(`Current version is not valid SemVer: '${currentVersion}'`);
return;
} else if (!repoUrl) {
exitWithError(
`npm package repository URL not found. Please set the '--repo' flag, or run this as an npm script from a project with the 'repository' field set.`,
);
return;
} else if (!isValidUrl(repoUrl)) {
exitWithError(`Invalid repo URL: '${repoUrl}'`);
return;
}
return exitWithError(
`Version not found. Please set the --currentVersion flag, or run this as an npm script from a project with the 'version' field set.`,
);
}
if (semver.valid(currentVersion) === null) {
return exitWithError(`Current version is not valid SemVer: '${currentVersion}'`);
}
if (!repoUrl) {
return exitWithError(
`npm package repository URL not found. Please set the '--repo' flag, or run this as an npm script from a project with the 'repository' field set.`,
);
}
if (!isValidUrl(repoUrl)) {
return exitWithError(`Invalid repo URL: '${repoUrl}'`);
}

Copy link
Member Author

@Gudahtt Gudahtt Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The pattern of placing the return on the following line was to make complying with the ESLint "consistent-return" rule easier.

Now that you're pointing it out, I think I agree that this style is harder to read and more error prone. I can adjust that in a follow-up PR and find a better solution for complying with that lint rule. I'd rather avoid doing it here because the same pattern is used in more places, and I'd like to change them all at once. It's distinct from the main purpose of this PR as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in #122


let changelogPath = changelogFilename;
if (!path.isAbsolute(changelogFilename) && projectRootDirectory) {
changelogPath = path.resolve(projectRootDirectory, changelogFilename);
Expand Down