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

Fixed documentation typos for issue #2925 #2934

Merged
merged 3 commits into from Jul 9, 2021

Conversation

itsdeekay
Copy link

@itsdeekay itsdeekay commented Jul 8, 2021

Hi

I have fixed the typo as suggested in issue resolves #2925 . Please review it and merge it.

@itsdeekay itsdeekay changed the title Fixed documentation typos for issue 2925 Fixed documentation typos for issue #2925 Jul 8, 2021
Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Thank you for your swift action.

It seems that your editor has automatically "tidied" the code. Unfortunately, that changes what could have been a single-line review into a >1k line review. I made a start at looking through each change (see comments below), but most of the changes seemed rather arbitrary and I think looking through all of them is not worth the time.

Please switch off the auto-tidy in your editor, commit just the fix, and then force-push.

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Much better now! Please add the fix to the _.unescape documentation as well. You also made a typo, see comment below.

index.html Outdated Show resolved Hide resolved
Copy link
Collaborator

@jgonggrijp jgonggrijp left a comment

Choose a reason for hiding this comment

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

Excellent! Just one final request, in order to clean up the commit history a bit. Could you please execute the following commands on your issue-2925 branch:

git branch backup-2925
git rebase --onto master c086b4392
git diff backup-2925

The diff should be empty. If it is, please finish with the following commands:

git push --force
git branch -D backup-2925

The overall effect of these commands will be that the original commit with the prettier changes and its revert will be removed from the history.

@itsdeekay
Copy link
Author

I have performed the steps as you mentioned except the fourth which was giving error so I did this instead git push origin issue-2925 --force and created a pull request. Seems like commit history is still there. Is there something I missed?

@jgonggrijp
Copy link
Collaborator

GitHub sometimes still shows the old commits for a while after you've updated the branch by force-push. No worries, it seems everything went the way it should. You can see this for yourself in the "Commits" and "Files changed" tabs.

Merging now. Congratulations with your first contribution!

@jgonggrijp jgonggrijp merged commit f098f61 into jashkenas:master Jul 9, 2021
@itsdeekay
Copy link
Author

Thanks @jgonggrijp for your guidance. I hope to work on more problems. If you have any open work where I can work please let me know.

@jgonggrijp
Copy link
Collaborator

@itsdeekay You are welcome to work on any open issue that doesn't have the "contrib", "breaking change" or "fixed" label. (If it does have one of those labels, you're welcome too, but I'm much more likely to reject the PR.)

In addition, if you run into anything while using Underscore yourself, feel free to submit a new issue ticket and/or submit a PR to address it.

@itsdeekay
Copy link
Author

That's the case, most of the issues are related to these flags only and others are already assigned or are discussion pending. I am open to work on other open source projects if there is requirement. Thanks

@jgonggrijp
Copy link
Collaborator

That still leaves at least #2909, #2853 and #2780. If you're up to the challenge, you can also ignore my assignment and try your hand at #2927. If none of that suits you, I can also use help in https://github.com/documentcloud/underscore-contrib, for example documentcloud/underscore-contrib#133 or any of the issues with a "docs" label.

@itsdeekay
Copy link
Author

Thanks, created pull request for #2780

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.

Documentation _.unescape uses wrong hex code
2 participants