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

Rdoc enhancements #413

Merged
merged 4 commits into from
Jun 27, 2020
Merged

Rdoc enhancements #413

merged 4 commits into from
Jun 27, 2020

Conversation

BurdetteLamar
Copy link
Member

Enhancements in json.rb:

  • Added JSON types.
  • More examples for JSON.parse and JSON.generate.
  • Examples for all JSON additions.
  • Example custom JSON addition.

Enhancements in common.rb:

  • More examples for JSON.generate, its options, and its exceptions.
  • More examples for JSON.parse, its options, and its exceptions.
  • Example for JSON.pretty_generate.

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.

@BurdetteLamar
Copy link
Member Author

@flori, I think this is ready for review.

@BurdetteLamar
Copy link
Member Author

@flori, you are not interested in this?

Copy link
Member

@marcandre marcandre left a 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

lib/json.rb Outdated Show resolved Hide resolved
lib/json.rb Outdated Show resolved Hide resolved
lib/json.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json/common.rb Outdated Show resolved Hide resolved
lib/json.rb Outdated Show resolved Hide resolved
@BurdetteLamar
Copy link
Member Author

BurdetteLamar commented Jun 25, 2020

@marcandre, if nothing more, I'll go ahead and merge this.
And thanks for your careful review.

@marcandre
Copy link
Member

@marcandre, if nothing more, I'll go ahead and merge this.

There are 6 unresolved comments.

And thanks for your careful review.

My pleasure. Great PR

@BurdetteLamar
Copy link
Member Author

@marcandre, Oops, I don't have merge access here (though I'm a member of ruby-committers).
Do you know who should look at this?

@marcandre
Copy link
Member

@marcandre, Oops, I don't have merge access here (though I'm a member of ruby-committers).
Do you know who should look at this?

Same here 😅

@hsbt will merge when ready

@marcandre
Copy link
Member

marcandre commented Jun 25, 2020

@BurdetteLamar would you please check the remaining 6 items?

@hsbt
Copy link
Member

hsbt commented Jun 25, 2020

I'm going to merge after approving by @marcandre

@BurdetteLamar
Copy link
Member Author

Thanks, much, to both of you!

I'm going to merge after approving by @marcandre

Copy link
Member

@marcandre marcandre left a 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.

@marcandre
Copy link
Member

@hsbt: LGTM, except that 4 commits should be squash-merged.

@BurdetteLamar
Copy link
Member Author

@hsbt, will you be merging this? I don't want to do anything more until this PR is resolved.

@hsbt hsbt merged commit 3d1c6a9 into ruby:master Jun 27, 2020
@hsbt
Copy link
Member

hsbt commented Jun 27, 2020

Merged.

@BurdetteLamar You should avoid to use RDoc enhancements each commits. There is no information for other people from its commit message. You write the description/how/why of your work on commits.

@marcandre
Copy link
Member

@BurdetteLamar You should avoid to use RDoc enhancements each commits. There is no information for other people. You write the description/how/why of your work on commits.

@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?

@hsbt
Copy link
Member

hsbt commented Jun 28, 2020

Why didn't you?

It's my mistake because this repository didn't set squash-merge as default.

I find the fact that you haven't written any positive message

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.

@hsbt
Copy link
Member

hsbt commented Jun 28, 2020

say "There is no information for other people" a bit ironic. There are 500 new lines of information...

I said it for commit message, not for this patch. I thanks to this contribute.

@marcandre
Copy link
Member

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'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?

@BurdetteLamar
Copy link
Member Author

@marcandre, maybe best just let @hsbt rest. Past doesn't matter. Let's just concern ourselves with the future?

This was referenced Mar 11, 2021
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