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

switch quoting library #491

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

switch quoting library #491

wants to merge 2 commits into from

Conversation

soletan
Copy link

@soletan soletan commented Jun 22, 2024

fixing #487

The PR is replacing the generic approach in shell-quote library with a different one based on character-by-character tests I've made resulting in a different quoting library.

@coveralls
Copy link

Coverage Status

coverage: 99.136% (+0.002%) from 99.134%
when pulling 47cba34 on cepharum:main
into 3dc909a on open-cli-tools:main.

@soletan
Copy link
Author

soletan commented Jun 22, 2024

Hm, not sure about the open linter errors for running ESLint on my working copy presents that warning, only. In case it is fixing something locally, this does not result in a change accepted for commission by git CLI tool.

Copy link
Member

@gustavohenke gustavohenke left a comment

Choose a reason for hiding this comment

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

not sure about the open linter errors

Maybe when updating pnpm lockfile eslint or prettier versions got updated, which now cause those?

Copy link
Member

Choose a reason for hiding this comment

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

Seems like a lockfile version bump here.
I'll try updating this on main first to decrease the LOC in this PR.

@@ -55,11 +55,11 @@
"author": "Kimmo Brunfeldt",
"license": "MIT",
"dependencies": {
"@cepharum/quoting-db": "^1.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

Seems like a fresh new package, would you be keen on hosting and maintaining it from the open-cli-tools org? Seems it'd fit perfectly ;)

@@ -10,6 +10,8 @@ export class ExpandArguments implements CommandParser {
constructor(private readonly additionalArguments: string[]) {}

parse(commandInfo: CommandInfo) {
const configuration = getShellConfigurationSync();
Copy link
Member

Choose a reason for hiding this comment

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

Looked at the source of this function briefly, seems it defaults to the value of COMSPEC.
Since cmd.exe is hardcoded for windows (source below), should the same be done here?

if (process.platform === 'win32') {
file = 'cmd.exe';

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.

3 participants