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

Document how and when the library raises exceptions #1412

Merged
merged 5 commits into from
Apr 19, 2022

Conversation

timrogers
Copy link
Contributor

This adds a section to the readme which explains when the library will raise exceptions and how you can work with them.

I did some work with octokit/octokit.rb and octokit/octokit.js recently, and I found that not having this meant that I had to do a bunch of digging myself to understand how I should handle unhappy paths.

As well as adding the error handling documentation, this PR also makes a few other small changes to the README: correcting, renaming and restructuring headings.

The Table of Contents in the readme has the "SSL Connection Errors"
section as its own heading, when it should be under "Working with
GitHub Enterprise". This fixes it.
@timrogers timrogers force-pushed the timrogers/readme-errors branch from 6b225d9 to ec45c90 Compare April 14, 2022 11:23
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

Hey @timrogers thanks for these additions! I just left one small nit on the bulleted items for the errors. Let me know what you think!

README.md Outdated Show resolved Hide resolved
@timrogers timrogers force-pushed the timrogers/readme-errors branch from 3c59430 to 97a26f5 Compare April 14, 2022 14:28
Copy link
Contributor

@nickfloyd nickfloyd left a comment

Choose a reason for hiding this comment

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

👍 LGTM! Side note - did you intend your base branch to be octokit:4-stable - This feels like the base should be master given it's just a docs change and not a hotfix.

@indigok
Copy link

indigok commented Apr 14, 2022

👍 LGTM! Side note - did you intend your base branch to be octokit:4-stable - This feels like the base should be master given it's just a docs change and not a hotfix.

octokit:4-stable is still the correct branch! master has significantly diverged at this point 😄

@timrogers
Copy link
Contributor Author

I'm not allowed to merge this, which I'm guessing is due to the outstanding GitHub checks (which aren't needed due to the ci skip!).

I'll just let the checks run for the last commit to get around this 🙏

@timrogers
Copy link
Contributor Author

octokit:4-stable is still the correct branch! master has significantly diverged at this point 😄

Interesting! 🤔 Is there some clean-up we can or should do here? Perhaps we could delete master and create a new main branch starting from 4-stable?

@nickfloyd
Copy link
Contributor

Interesting! 🤔 Is there some clean-up we can or should do here? Perhaps we could delete master and create a new main branch starting from 4-stable?

Yeah, we've got plans to get things cinched up around builds, workflow, and CI. Great work has already been done on octokit Ruby; it will take a bit of effort, but I am definitely looking forward to tackling things like these so that we can get this to a point where it is up to date and more welcoming to contributors!

Thanks @nickfloyd 🥰!

Co-authored-by: Nick Floyd <139819+nickfloyd@users.noreply.github.com>
@timrogers timrogers force-pushed the timrogers/readme-errors branch from 97a26f5 to 7b17087 Compare April 14, 2022 20:14
@timrogers
Copy link
Contributor Author

Interesting! 🤔 Is there some clean-up we can or should do here? Perhaps we could delete master and create a new main branch starting from 4-stable?

Yeah, we've got plans to get things cinched up around builds, workflow, and CI. Great work has already been done on octokit Ruby; it will take a bit of effort, but I am definitely looking forward to tackling things like these so that we can get this to a point where it is up to date and more welcoming to contributors!

Wonderful! 🙌🏻

@timrogers
Copy link
Contributor Author

@nickfloyd This should be ready for merge now. Thank you so much!

@imwiss
Copy link
Contributor

imwiss commented Apr 19, 2022

Thanks for updating this, @timrogers! ✨

@imwiss imwiss merged commit 676d31f into octokit:4-stable Apr 19, 2022
@nickfloyd nickfloyd added Type: Documentation Improvements or additions to documentation and removed docs-and-samples labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants