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.

Sign up for GitHub

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

Jump to bottom

Improve code coverage of src/info/repo/commits.rs #813

Merged
merged 1 commit into from
Oct 10, 2022
Merged

Improve code coverage of src/info/repo/commits.rs #813

merged 1 commit into from
Oct 10, 2022

Conversation

alessandroasm
Copy link
Contributor

Part of task #700

@alessandroasm
Copy link
Contributor Author

Not sure why Github is showing this as a full rewrite of the file :(

@spenserblack
Copy link
Collaborator

Not sure why Github is showing this as a full rewrite of the file :(

Did the line endings change? LF to CRLF? Not sure which editor you use, but it will likely show you if you're using Unix/LF or Windows/Dos/CRLF.

spenserblack
spenserblack requested changes Oct 10, 2022
View reviewed changes
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

Marking this as needing changes since we should probably fix the diff before merging.

This was referenced Oct 10, 2022
Merged
Merged
@alessandroasm
Copy link
Contributor Author

Great, I'm gonna wait until #815 gets merged and then I'll update / rebase this if needed. Thank you @spenserblack

o2sh added a commit that referenced this pull request Oct 10, 2022
@spenserblack @o2sh
Convert line endings to LF (#815)
7f37977 
* Convert line endings to LF

This should result in more reasonable diffs of future contributions
where a contributor's text editor would automatically convert line
endings.

See #812, #813

* Create .rustfmt.toml

Enforces Unix-style newlines.

Co-authored-by: Ossama Hjaji <ossama-hjaji@live.fr>

Co-authored-by: Ossama Hjaji <ossama-hjaji@live.fr>
@spenserblack
Copy link
Collaborator

Thanks @alessandroasm! If you feel comfortable rebasing onto main then can you please do so?
There's a good chance that this might be squash-merged, so a normal merge commit is fine, too.

@spenserblack
Copy link
Collaborator

P.S. git checkout --ours and git checkout --theirs are useful when resolving these full-file conflicts.

@alessandroasm
Copy link
Contributor Author

Thanks for all the help @spenserblack! It should be OK now 👍

Please let me know if there's anything else I could improve.

spenserblack
spenserblack approved these changes Oct 10, 2022
View reviewed changes
Copy link
Collaborator

@spenserblack spenserblack left a comment

Choose a reason for hiding this comment

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

This diff looks much better 😄

@o2sh o2sh merged commit c88541d into o2sh:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spenserblack spenserblack spenserblack approved these changes

@o2sh o2sh Awaiting requested review from o2sh o2sh is a code owner

@Byron Byron Awaiting requested review from Byron

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alessandroasm @spenserblack @o2sh