Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Allow using lone arguments #851

Closed
sirenkovladd opened this issue Nov 12, 2024 · 7 comments
Closed

Allow using lone arguments #851

sirenkovladd opened this issue Nov 12, 2024 · 7 comments

Comments

@sirenkovladd
Copy link

sirenkovladd commented Nov 12, 2024

example

// ✅
poku --config=my-file.json
// lone argument
poku --config my-file.json
// multiple arguments
poku --filter .test --filter .spec
// group of shorts
poku -pqdw

// short and value
poku -cmy-file.json // -> poku --config=my-file.json

maybe, a breaking change for env
can be moved as part of #801

More details:
https://github.com/nodejs/node/blob/main/lib/internal/util/parse_args/parse_args.js#L217-L300

@wellwelwel
Copy link
Owner

wellwelwel commented Nov 12, 2024

Hi, @sirenkovladd 🙋🏻‍♂️

In Poku's current design, this would cause a conflict:

Currently, Poku filters out all values passed via CLI that don't start with - or -- and understands that they are paths.
Also, maintaining both forms would be unhealthy for good long-term maintenance.

There is also a particular reason why I chose to use the flag=value idea, which was the difficulty in maintaining support for Windows.


About mixing short flags and values, are there any particular advantages to this?

For example, we would have to split the first character from the rest to define who the parameter is and what the value is, currently we don't need to validate the parameter unless we use the -x flag.

Also, it would cause a redundant check because when combining short flags, we would have to understand that for this specific case, we shouldn't split the first character from the rest, but all the characters.


Now, as for the combination of short flags and the possibility of multiple use of the same flags with different values, those would be great features ✨

  • With the exception of these two, I wonder how much trouble these features are worth for any new flag that is created in the future, as well as how much this could compromise performance due to the need for redundancy checks.

Please, let me know what you think 💡

@sirenkovladd
Copy link
Author

I'm a little confused, what is the complexity --config my-file.json the syntax for Windows?

As for short and value, I think it's a standard, for example date you can use argument -r -> date -r1533415339

@wellwelwel
Copy link
Owner

wellwelwel commented Nov 13, 2024

I'm a little confused, what is the complexity --config my-file.json the syntax for Windows?

I'm not a Windows user, so it's natural for me think first in Unix solutions and approaches. For example, until #80 I didn't know Poku didn't work with Windows 😂

There is no specific limitation, but a question of balancing compatibility, functionality and long-term ease of maintenance (I'll explain more about this below).


As for short and value, I think it's a standard, for example date you can use argument -r -> date -r1533415339

One of Poku's intentions is to be similar to Node.js at key points, for example, Node.js follows the flag=value idea:

node --test-reporter=spec --test-reporter=dot --test-reporter-destination=stdout --test-reporter-destination=file.txt 

At the same time, it's really easier to maintain support, as we can simply split the = into a flag and value.
Avoiding too many redundant checks is also crucial when it comes to performance.

If the idea is to implement a robust and complete CLI tool, we could use yargs or a similar tool, but Poku has no intention of replacing robust tools like this, for example. And the idea is to keep Poku free of external dependencies.

So, how it works?

As mentioned above, there is a balance between creating a feature from scratch that ensures a good balance between actually working for what is needed, being easy to support in the long term, but at the same time being minimalist so that Poku doesn't lose its focus of being a test runner.

Why do I emphasize on long-term support?

The project has had various external contributions, but the majority of the time, I tend to maintain the project alone. But I'm hopeful that this will still change ✨

Creating too many complex strands that don't derive from the main topic of a test runner can cause an overload that will make it increasingly difficult to maintain and even create new features.

Priorities

Priorities and focus are always on the functionalities that a test runner really needs, but that doesn't mean that I don't intend to improve functionalities that add value for the end user.

With this in mind, I believe it's of great value for Poku to be able to combine short-flags, for example.


Don't take what I say as a rule, I love to discuss and talk 💡

@sirenkovladd
Copy link
Author

Okay, I agree that the node philosophy does not include short and value

but lone argument seems to be supported

node --eval "console.log(123)"
node --require tsx/cjs ./file.ts
node --env-file .env index.js

@wellwelwel
Copy link
Owner

wellwelwel commented Nov 13, 2024

Okay, I agree that the node philosophy does not include short and value

but lone argument seems to be supported

node --eval "console.log(123)"

node --require tsx/cjs ./file.ts

node --env-file .env index.js

Considering the current way Poku works and that the focus is not on creating a robust CLI tool equivalent to yargs, for example, how would you suggest maintaining support for both formats without compromising performance (especially when extending Glob patterns)?

A honest question: do you believe that the future work of maintaining dual compatibility for a side CLI helper is worth the benefit it would bring instead of focusing on the aggregate functionalities of a test runner?

@sirenkovladd
Copy link
Author

I think that if you go the way it is implemented in Node.js, should have a list of arguments and their type, if this type is a string and is not a union argument (--env=some-file), then just take the next argument as a value (--env some-file)

this is not a problem for most arguments, except when the argument supports both boolean type a and string (env, denoCjs,only)


I don't think it is dual compatibility
it seems more like the expected behavior when someone interacts via cli

I agree that the current version is more readable, but honestly all my cli commands are as lone argument (--require tsx/cjs)

@wellwelwel wellwelwel changed the title allow using lone arguments Allow using lone arguments Nov 13, 2024
@wellwelwel
Copy link
Owner

For everyone.

Although I've added the "help wanted" label, I'll point out a few important aspects and suggest taking care before getting hands-on with the code:

  • Adding this functionality in combination with double compatibility will mean double testing for each variation.
  • How should we deal with the user who mixes both uses at the same time for the same command?
  • Should we abandon the philosophy of maintaining Poku without external dependencies and accept an appropriate tool to parse the CLI?
  • Or should we drop support for Node.js versions 12 and 14 and use the native CLI tool available from version 16?

About the tests

Using the --denoCjs feature as an example, teríamos que aceitar e testar:

  • --denoCjs (true)
  • --denoCjs=js
  • --denoCjs=js,cjs
  • --denoCjs js
  • --denoCjs js,cjs or --denoCjs js cjs or --denoCjs js --denoCjs cjs

This would triple the possibility of testing each feature via CLI (existing or new).

Currently, these are the CLI options available (26):

poku/src/bin/help.ts

Lines 12 to 39 in 26fcf0e

const summary: [string, string][] = [
['--bun', 'Enforce tests to run through Bun.'],
['--concurrency', 'Limit the number of tests running concurrently.'],
['--config, -c', 'Specify a configuration file.'],
['--debug, -d', 'Show detailed logs.'],
['--deno', 'Enforce tests to run through Deno.'],
['--denoAllow', 'Allow permissions for Deno.'],
['--denoCjs', 'Support CommonJS in Deno.'],
['--denoDeny', 'Deny permissions for Deno.'],
['--enforce, -x', 'Validate options before running tests.'],
['--envFile', 'Read and set an environment file.'],
['--exclude', 'Exclude by path using Regex to match files.'],
['--failFast', 'Stop tests at the first failure.'],
['--filter', 'Filter by path using Regex to match files.'],
['--help, -h', "Show Poku's CLI basic usage."],
['--killPid', 'Terminate the specified processes.'],
['--killPort', 'Terminate the specified ports.'],
['--killRange', 'Terminate the specified port ranges.'],
['--listFiles', 'Display all the files returned in the terminal.'],
['--node', 'Enforce tests to run through Node.js.'],
['--only', 'Enable selective execution of tests.'],
['--parallel, -p', 'Run tests files in parallel.'],
['--platform', 'Enforce tests to run through a platform.'],
['--quiet, -q', 'Run tests with no logs.'],
['--version, -v', "Show Poku's installed version."],
['--watch, -w', 'Watch for test events.'],
['--watchInterval', 'Set an interval for watch events.'],
];

As for the point that Node.js also accepts both formats, it's still difficult for me to understand the real advantage of migrating to the second standard.

What would be the benefits of this approach for the end user?


About the documentation

  • Should the documentation be adapted to suggest both formats?
  • Should we address only one of the formats in the documentation, even if both are supported?

My personal opinion

I'm in favor of maintaining support for only one of the formats:

  • --flag=value or --flag value (breaking change).

But most importantly, Poku is not a CLI tool and is not intended to be. With this in mind, what would be the advantages of doubling or tripling the complexity of project maintenance in the long term (more test variations and more complex documentation)?


All opinions and discussions are welcome in order to clarify how worthwhile it is to invest in this functionality that is not directly related to the scope of a test runner 🤝

Note

Moving from issue to discussion.

Repository owner locked and limited conversation to collaborators Nov 13, 2024
@wellwelwel wellwelwel converted this issue into discussion #853 Nov 13, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests

2 participants