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

test: add failed test and edge case #155

Conversation

Sebastien-Ahkrin
Copy link

@Sebastien-Ahkrin Sebastien-Ahkrin commented Oct 31, 2023

Problems

We should allow user's to pass big string on ace commands without making them numbered.
For example => 111111111111111111111111 should still be "111111111111111111111111" after getopt output.
(Actually getopt transform this to a number).

A simple fix should be to add the flag on the string option of getopt. But, if we do that, we have some bug on the application. Because, getopt transform empty flag to empty string

getopt("--str=111111111111111111111111", { string: ["str", "test"] })

will return something like:

{
  "str": "111111111111111111111111",
  "test": ""
} 

An issue about something like that already exist here : jorgebucaran/getopts#53

I added a failed test, who explain the "problems" we found on ace

@thetutlage
Copy link
Member

Cool, I see that.

Can you please update the same PR and provide the fix as well?

@Sebastien-Ahkrin
Copy link
Author

Cool, I see that.

Can you please update the same PR and provide the fix as well?

I'm sorry, actually i don't have any fix. A "simple" fix may be to change the "getopt" packages by yargs or something like that. What do you think about that @thetutlage ?

@thetutlage
Copy link
Member

@Sebastien-Ahkrin

We have moved from getopts to yargs in the newest version of Ace (with breaking changes). The newer version will be used with v6.

So is it okay for now to sit on this bug? Or do you need it to be fixed for v5 apps too?

@Sebastien-Ahkrin
Copy link
Author

Sebastien-Ahkrin commented Nov 6, 2023

@Sebastien-Ahkrin

We have moved from getopts to yargs in the newest version of Ace (with breaking changes). The newer version will be used with v6.

So is it okay for now to sit on this bug? Or do you need it to be fixed for v5 apps too?

I think its okay for us so if the bug isn't here on v6. Thanks so!
If we want to fix it, do we can change getopt to yargs on v5 on a PR?

@thetutlage
Copy link
Member

I think it will be a lot complicated to swap getopts with yargs and might introduce breaking changes as well. So, it will be nice to leave the current version as it is and then use the latest version of ace in coming months.

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.

2 participants