-
Notifications
You must be signed in to change notification settings - Fork 0
TypeScript Rewrite: Get-version command #34
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: ENG-219/app-bootstrap
Are you sure you want to change the base?
Changes from 11 commits
0a4ecd3
8c2c95a
529866d
b063ed1
6649e64
46f18fb
2912a4f
58b2263
7191246
524fe69
7506e7b
8488462
41a4c47
d99bf89
65f3160
029df3b
b9f7d7d
aba8600
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import type { Command } from 'commander'; | ||
| import fs from 'fs-extra'; | ||
| import path from 'node:path'; | ||
| import { getConfig } from '../config.js'; | ||
| import { runCommandSilent } from '../utils/process.js'; | ||
| import * as output from '../utils/output.js'; | ||
|
|
||
| /** | ||
| * Extracts the project version from the first configured version file. Appends a dev suffix | ||
| * with git timestamp and hash when the dev option is set. | ||
| * | ||
| * @since TBD | ||
| * | ||
| * @param {object} options - The options object. | ||
| * @param {boolean} [options.dev] - Whether to append a dev suffix to the version. | ||
| * @param {string} [options.root] - The root directory for resolving version files. | ||
| * | ||
| * @returns {Promise<string>} The resolved version string, or 'unknown' if not found. | ||
| */ | ||
| export async function getVersion(options: { | ||
| dev?: boolean; | ||
| root?: string; | ||
| }): Promise<string> { | ||
| const config = getConfig(options.root); | ||
| const versionFiles = config.getVersionFiles(); | ||
| const cwd = options.root ?? config.getWorkingDir(); | ||
|
|
||
| if (versionFiles.length === 0) { | ||
| return 'unknown'; | ||
| } | ||
|
|
||
| const versionFile = versionFiles[0]; | ||
| const filePath = path.resolve(cwd, versionFile.file); | ||
| const contents = await fs.readFile(filePath, 'utf-8'); | ||
| const regex = new RegExp(versionFile.regex); | ||
| const matches = contents.match(regex); | ||
|
|
||
| let version: string; | ||
| if (!matches || !matches[2]) { | ||
| version = 'unknown'; | ||
| } else { | ||
| version = matches[2]; | ||
| } | ||
|
|
||
| if (options.dev) { | ||
| const timestampResult = await runCommandSilent( | ||
| 'git show -s --format=%ct HEAD', | ||
| { cwd } | ||
| ); | ||
| const hashResult = await runCommandSilent( | ||
| 'git rev-parse --short=8 HEAD', | ||
| { cwd } | ||
| ); | ||
| const timestamp = timestampResult.stdout.trim(); | ||
| const hash = hashResult.stdout.trim(); | ||
| version += `-dev-${timestamp}-${hash}`; | ||
| } | ||
|
|
||
| return version; | ||
| } | ||
|
|
||
| /** | ||
| * Registers the `get-version` command with the CLI program. | ||
| * | ||
| * @since TBD | ||
| * | ||
| * @param {Command} program - The Commander.js program instance. | ||
| * | ||
| * @returns {void} | ||
| */ | ||
| export function registerGetVersionCommand(program: Command): void { | ||
| program | ||
| .command('get-version') | ||
| .description('Gets the version for the product.') | ||
| .option('--dev', 'Get the dev version.') | ||
| .option('--root <dir>', 'Set the root directory for running commands.') | ||
| .action(async (options: { dev?: boolean; root?: string }) => { | ||
| const version = await getVersion(options); | ||
| output.writeln(version); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import type { VersionFile } from '../types.js'; | ||
|
|
||
| /** | ||
| * Creates a VersionFile object from a file path and regex pattern. | ||
| * | ||
| * @since TBD | ||
| * | ||
| * @param {string} file - The file path. | ||
| * @param {string} regex - The regex pattern. | ||
| * | ||
| * @returns {VersionFile} A VersionFile object with the provided file path and regex pattern. | ||
| */ | ||
| export function createVersionFile(file: string, regex: string): VersionFile { | ||
| return { file, regex }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { | ||
| runPup, | ||
| writePuprc, | ||
| getPuprc, | ||
| createTempProject, | ||
| cleanupTempProjects, | ||
| } from '../helpers/setup.js'; | ||
|
|
||
| describe('get-version command', () => { | ||
| let projectDir: string; | ||
|
|
||
| beforeEach(() => { | ||
| projectDir = createTempProject(); | ||
| writePuprc(getPuprc(), projectDir); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| cleanupTempProjects(); | ||
| }); | ||
|
|
||
| it('should get the version from the project', async () => { | ||
| const result = await runPup('get-version', { cwd: projectDir }); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout).toContain('1.0.0'); | ||
| }); | ||
|
|
||
| it('should get the dev version from the project', async () => { | ||
| const result = await runPup('get-version --dev', { cwd: projectDir }); | ||
| expect(result.exitCode).toBe(0); | ||
| expect(result.stdout).toContain('1.0.0'); | ||
| expect(result.stdout).toContain('dev'); | ||
|
||
| }); | ||
| }); | ||
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.
I so dislike this API.
First question: why are version files an array, and why do we just always use the first item? We should either change this (with soft deprecation) and make it an object, or test every pair individually and use the first successful one. Its espesially odd to have that fixture here -
pup/tests/helpers/setup.ts
Lines 19 to 37 in 741d07d
Second, the requirement to have exactly two capturing groups is super-odd. Why don't we use named capture groups? It's supported with PHP and JS (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Regular_expressions/Named_capturing_group and https://www.php.net/manual/uk/function.preg-match.php#example-4) with compatible syntax
(?<name>pattern). Couldn't we agree on a(?<version>pattern)named group that should represent the version string? We can also can soft deprecate that oddmatches[2]here and in the config check.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 is for
pup check:version-conflictprimarily. It helps ensure that if you have the version defined in multiple places (such as the WP Plugin Header, a Constant, a version listed inreadme.txt, etc.) that they're correctly updated. That check will ensure they match.That does seem to be a strange choice for
pup get-versionspecifically. I've updated it so that it will look through them all until it finds a successful match, and then it will return the first match.If no configured version files are configured, then it will now throw an Error. If there are configured Version files and it cannot find a version in any of them, that will also now throw an Error.
I've implemented this now :) It'll try for a named capture group first, then fall back to the second capture group. That was a weird behavior, but it was how the PHP version worked 🤷
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.
Weirdly,
get-versionhas a--rootflag and it isn't clear what the use case for it would be. We probably should just remove it. It existed in the PHP version, but I don't know why you would want to callpup get-version --root some-dirto change the relative path for the version files when checking 🤔I've tested it a bit and Doing something like:
And running
cd src/ bunx pup -- get-version --root ../ php pup.phar -- get-version --root ../doesn't work at all, and something like that is the only use case I could think of for this.
I'm going to remove that option flag from this command to prevent that confusion.
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 only way it would make sense would be if it did change the root for the Config. But since that's not how the other use cases utilize it, I think that making it work by changing where the Config was loaded from for this command would be confusing.