-
Notifications
You must be signed in to change notification settings - Fork 332
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
Rdoc enhancements #413
Rdoc enhancements #413
Conversation
@flori, I think this is ready for review. |
@flori, you are not interested in this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 This is a fabulous upgrade to the docs 🙇♂️
Here's my initial review
@marcandre, if nothing more, I'll go ahead and merge this. |
There are 6 unresolved comments.
My pleasure. Great PR |
@marcandre, Oops, I don't have merge access here (though I'm a member of ruby-committers). |
Same here 😅 @hsbt will merge when ready |
@BurdetteLamar would you please check the remaining 6 items? |
I'm going to merge after approving by @marcandre |
Thanks, much, to both of you!
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Should be "squash and merged" into a single commit.
@hsbt: LGTM, except that 4 commits should be squash-merged. |
@hsbt, will you be merging this? I don't want to do anything more until this PR is resolved. |
Merged. @BurdetteLamar You should avoid to use |
@hsbt why didn't you squash-merge these commits? I find the fact that you haven't written any positive message or thank you about this 600+ lines PR and say "There is no information for other people" a bit ironic. There are 500 new lines of information... Yes, ideally these would have been squashed but Github gives you the possibility to do it yourself. Why didn't you? |
It's my mistake because this repository didn't set squash-merge as default.
Sorry, I have no enough english skill. But I also got it from you. I always feel blame, enforce and rush about fix, resolve, review and release each OSS repository in holidays and weekend. I'm tired about this. |
I said it for commit message, not for this patch. I thanks to this contribute. |
I'm sorry you feel this way. I am not aware of my comments that make you rush about anything. Could you please point out which of my comments made you feel rushed to do something? |
@marcandre, maybe best just let @hsbt rest. Past doesn't matter. Let's just concern ourselves with the future? |
Enhancements in json.rb:
Enhancements in common.rb:
Not enhanced: JSON.restore, JSON.dump, JSON.load, JSON.recurse_proc.
If preferred, I can put up a PR with just json.rb.
Also if preferred, I can put up piecemeal PRs for common.rb.