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
Contributor

@BurdetteLamar BurdetteLamar commented May 6, 2020

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
Contributor Author

BurdetteLamar commented May 10, 2020

@flori, I think this is ready for review.

@BurdetteLamar
Copy link
Contributor Author

BurdetteLamar commented Jun 3, 2020

@flori, you are not interested in this?

Copy link
Contributor

@marcandre marcandre left a comment

👍 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
Contributor 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
Contributor

marcandre commented Jun 25, 2020

@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
Contributor Author

BurdetteLamar commented Jun 25, 2020

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

marcandre commented Jun 25, 2020

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

marcandre commented Jun 25, 2020

@BurdetteLamar would you please check the remaining 6 items?

@hsbt
Copy link
Collaborator

hsbt commented Jun 25, 2020

I'm going to merge after approving by @marcandre

@BurdetteLamar
Copy link
Contributor Author

BurdetteLamar commented Jun 25, 2020

Thanks, much, to both of you!

I'm going to merge after approving by @marcandre

Copy link
Contributor

@marcandre marcandre left a comment

Great work! Should be "squash and merged" into a single commit.

@marcandre
Copy link
Contributor

marcandre commented Jun 25, 2020

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

@BurdetteLamar
Copy link
Contributor Author

BurdetteLamar commented Jun 27, 2020

@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 flori:master Jun 27, 2020
1 check passed
@hsbt
Copy link
Collaborator

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
Contributor

marcandre commented Jun 28, 2020

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

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
Collaborator

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
Contributor

marcandre commented Jun 28, 2020

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
Contributor Author

BurdetteLamar commented Jun 28, 2020

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

This was referenced Mar 14, 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.

None yet

3 participants