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

Move timeouts and rate-limiting to separate docs chapter #122

Merged
merged 5 commits into from
Dec 9, 2022

Conversation

JOJ0
Copy link
Contributor

@JOJ0 JOJ0 commented Dec 6, 2022

What do you think?

Any better ideas for a chapter name? Advanced Configuration? Settings? Advanced Settings?

What I don't like is that it looks pretty similar to "Contribution" since that is right next to it. Maybe just another position in the menu would fix that?

Suggestions welcome.
Cheers!

@JOJ0
Copy link
Contributor Author

JOJ0 commented Dec 6, 2022

image

@AnssiAhola
Copy link
Contributor

AnssiAhola commented Dec 6, 2022

Would it make any difference if we renamed Contribution to Contributing?

image

Other than that I personally like the Configuration
Also you mentioned moving it to other position, I think it would make more sense to have Configuration towards the top, maybe after Quickstart or Authentication ?

Copy link
Contributor

@prcutler prcutler left a comment

Choose a reason for hiding this comment

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

A couple notes:

I agree that it should be "Contributing". I also like the idea of moving it up after Quickstart and before Authentication.

I'd like to reword line 22, something like:

For example, to enable a request timeout of 5 seconds:

or something like that, because right after the example you did a great job of explaining what values are supported. At a minimum, I would recommend adding a colon.

@alifhughes
Copy link
Contributor

My 2 cents..

I agree with "Contributing". As for the configuration title, I was thinking as both as these are optional configurations, we could call it "Optional Configuration"?

I would keep it after "Authentication" though as auth is one of the most common questions/issues/requests we've had.

Other than thatm I agree with @prcutler's rewording too.

Hope that helps!

@JOJ0
Copy link
Contributor Author

JOJ0 commented Dec 8, 2022

https://python3-discogs-client.readthedocs.io/en/docs_config_chapter/

have a look. Enabled the branch (hidden just with link).

What do you think? Kept the position but renamed to optional. Reworded the "timeout sentence".

docs/source/index.rst Outdated Show resolved Hide resolved
@AnssiAhola
Copy link
Contributor

AnssiAhola commented Dec 8, 2022

Note sure what's up with the tests, maybe change ubuntu-latest to some specific version in https://github.com/joalla/discogs_client/blob/eba0bff930dd36f2583d4de67b002ad69bbfcf20/.github/workflows/discogs_client-build.yml

actions/setup-python#555

@JOJ0 JOJ0 force-pushed the docs_config_chapter branch 2 times, most recently from 03a5523 to 4b7a6c3 Compare December 9, 2022 08:11
@JOJ0
Copy link
Contributor Author

JOJ0 commented Dec 9, 2022

Thanks for the hint Anssi! Fixed!

File name was intentionally just configuration.md instead of longer, but you're right, probably it could be confusion when quickly trying to find it by beginning later. Will fix that and merge!

by using a specific Ubuntu version. Reason:
actions/setup-python/issues/555
optional_configuration.md
Copy link
Contributor

@AnssiAhola AnssiAhola left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@JOJ0 JOJ0 merged commit 87a5169 into master Dec 9, 2022
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.

4 participants