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

Workaround to make --no-newline work #23

Merged
merged 3 commits into from
Sep 14, 2021

Conversation

msabramo
Copy link
Contributor

@msabramo msabramo mentioned this pull request Sep 13, 2021
@Qix-
Copy link
Member

Qix- commented Sep 13, 2021

Does line 64 need to be changed?

@msabramo
Copy link
Contributor Author

msabramo commented Sep 14, 2021

@qix:

Does line 64 need to be changed?

You're referring to this line:

noNewline: {

?

Do you think this line should change? I'm not seeing a reason why, but maybe you're seeing something that I am not...?

@msabramo
Copy link
Contributor Author

I added a new test for the --no-newline case in a59ac22, so hopefully the tests make the case that everything is working as desired.

@@ -83,7 +83,7 @@ function init(data) {

const fn = dotProp.get(chalk, styles.join('.'));
process.stdout.write(fn(data.replace(/\n$/, '')));
if (!cli.flags.noNewline) {
if (!cli.flags.noNewline && cli.flags.newline !== false) {
Copy link
Member

Choose a reason for hiding this comment

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

This could use a short code comment explaining the logic and linking to the yarns-parser issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah okay, but I didn't create a yargs-parser issue yet because I found a possible way to do it using an existing boolean-negation feature in yargs-parser.

See #16 (comment)

That approach wouldn't require changing yargs-parser and would only require a small change to meow.

I thought that might be a pretty good approach, since it seemed to require only a small change to meow and since meow is one of your projects, it would be easy to get done.

Check out #16 (comment) and see if you like that, or if you think it's better to request a feature from yargs-parser.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sindresorhus: See 5e680e1. It adds a comment that explains and points the reader to #30

We can discuss things in that issue and/or link to any meow or yargs-parser issues or PRs that are created.

Copy link
Member

Choose a reason for hiding this comment

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

I would prefer an option in yarns-parser. That would benefit all consumers using yarns-parsers and not just meow. It seems like it is something that would be generally useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Would you please copy this comment to #30 ?

I think it will be nice to have it in an open issue so it is more likely to get eyeballs.

and referencing issue created to track simplifying it:

chalk#30
@Qix-
Copy link
Member

Qix- commented Sep 14, 2021

@sindresorhus are we still waiting for feedback or is this good to go?

@sindresorhus
Copy link
Member

@sindresorhus sindresorhus merged commit ea08e03 into chalk:bare-nl Sep 14, 2021
@msabramo
Copy link
Contributor Author

🎉

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.

None yet

3 participants