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

Option for npm diff to ignore cr at eol #522

Closed
wants to merge 2 commits into from

Conversation

oBusk
Copy link
Contributor

@oBusk oBusk commented Feb 4, 2022

An option to make npm diff more useful when working on packages developed
across multiple platforms.

This is my first suggestion. Please let me know if it needs clarification or
improvement!

References

@oBusk oBusk changed the title Intially create RFC Option for npm diff to ignore cr at eol Feb 5, 2022
@ruyadorno
Copy link
Contributor

Thanks for the contribution @oBusk! We chatted about it in our weekly OpenRFC calls (you're able to catch up with the recordings if you click on the linked issues here) and my feedback there is that this was the plan all along 😁 Launch with a minimal set of options enabled and let's wait for the community to surface what other options from git diff might be useful in npm diff. With that in mind I believe this is a natural RFC for us to accept.

It looks like you already got started on a PR in jsdiff to enable that (ref: kpdecker/jsdiff#344) it looks great and will make it very easy to add support to this option in npm.

I believe this is ready to land. So to answer to the questions at the end:

Option/flag name?

I'd stick with --diff-ignore-cr-at-eol as the npm config value and diffIgnoreCRAtEOL for internal libnpmdiff

Should ignoreAllSpace ignore CR?

I believe they're two different things but to be honest, we're probably just going to follow the default behavior that jsdiff provides, in case it changes there someday we would not do any extra work to avoid it.

Should CR be ignored by default?

I don't think so, it might be very useful to have that metadata surfaced in some cases and it seems to be a more "correct" default behavior. Anyone else might opt into the option in their .npmrc files.

@ruyadorno ruyadorno closed this in 25e4b08 Feb 23, 2022
@ruyadorno
Copy link
Contributor

Approved and landed in 25e4b08

Thank you so much @oBusk!

@oBusk
Copy link
Contributor Author

oBusk commented Feb 23, 2022

@ruyadorno that's so cool 😊 I had a look at both this and last weeks meeting and I agree with the notion of trying to keep npm diff to match git diff where possible. In the latest meeting it was mentioned a question of what the default in git diff is so I tried to have a look in the small test repo I setup and gnu diffutils behaved as I expected it, but honestly I can't get git diff to show CRLF differences but since the --ignore-cr-at-eol flag is not a toggleable option but a opt-in flag, I still assume the default behaviour is to not ignore CR vs CRLF differences 🤔

@ruyadorno
Copy link
Contributor

since the --ignore-cr-at-eol flag is not a toggleable option but a opt-in flag, I still assume the default behaviour is to not ignore CR vs CRLF differences

yeah, I'm assuming the same!

@darcyclarke darcyclarke removed the Agenda will be discussed at the Open RFC call label Apr 6, 2022
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