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

Edit docs - Add section with example of getting a user's inventory #123

Merged
merged 5 commits into from
Jan 22, 2023
Merged

Edit docs - Add section with example of getting a user's inventory #123

merged 5 commits into from
Jan 22, 2023

Conversation

lemonase
Copy link
Contributor

Problem

In regards to Marketplace Listings - there are examples for Create, Update and Delete but not Read.

I will admit that this is something that confused me at first, so I wanted to add examples for others who maybe trying to do the same.

Pull Request

Pull request to update the README and docs with examples of how you can read and get listings from a user's inventory without authenticating as said user.

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 18, 2023

Hi, awesome! Thanks for adding a missing piece to our docs. Greatly appreciated.

Please double check your commits if all the changes really where intentional. Beside the new chapter, I see several changes where you add < and > characters to hyperlinks. Why?

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 18, 2023

As a first time contributor I had to approve github actions workflows to run. I did that just now.

What you could also do to make sure the docs looks as intended: Build them locally. See docs page: https://python3-discogs-client.readthedocs.io/en/latest/writing_docs.html

The note about Py 3.10 issues might be outdated. Please report if your build went fine and I'll update this part of the docs.

@lemonase
Copy link
Contributor Author

Yes, thank you. The angle brackets around the links are from my editor doing autoformatting. It doesn't effect the rendering/output of the markdown on GitHub or the docs so either way works.

I was able to build the sphinx docs using Python 3.9.6 and 3.10.6 as well.

Copy link
Contributor

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

Regarding the < and > bracket characters: Can you provide information and documentation describing that this is proper markdown syntax? On a quick search I didn't find much. At least the discogs documentation suggests writing links without them: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#links. I suggest we leave them as they are as long as we don't have a good reason to change them.

Thanks a lot for reporting back that the dosc build works. We'll change the docs accordingly.

Some minor change requests within.

README.mkd Outdated Show resolved Hide resolved
README.mkd Outdated Show resolved Hide resolved
README.mkd Show resolved Hide resolved
docs/source/listing.md Show resolved Hide resolved
@lemonase
Copy link
Contributor Author

Regarding the < and > bracket characters: Can you provide information and documentation describing that this is proper markdown syntax? On a quick search I didn't find much. At least the discogs documentation suggests writing links without them: https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#links. I suggest we leave them as they are as long as we don't have a good reason to change them.

The only thing I can refer you to for this is the VS Code extension I had installed markdownlint
The extension is simply more opinionated/aggressive with formatting than others.

I can certainly revert these - as I didn't necessarily intend to make these changes in the first place.

Everything else concerning styling/consistency is fine as well.

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 20, 2023

Thanks for the changes. I'd prefer the links to be reverted. Quickly looked throught that linters specs but couldnt find anything, but was on mobile and probably not a thorough search: https://github.com/DavidAnson/markdownlint/blob/main/doc/Rules.md#md042---no-empty-links

In any case: Consistency throughout the docs is good, so better we leave them.

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 20, 2023

Guys @alifhughes @prcutler @AnssiAhola anything from your end when you look at the final diff or we ready to merge?

@AnssiAhola
Copy link
Contributor

Overall it looks fine to me, maybe the "unnecessary" code formatting (white space, links, etc) throws me off a bit, but I think it's fine as is.

About those links being converted by the linter, maybe it's triggered by this rule and it thinks that ( and ) around links are considered "bare url"?
Although this rule indicates that the original syntax was correct. hmm... a bug? 🤔

Anyway maybe we should make a separate PR where we go through the docs and make them consistent and "correct"? Maybe using this linter or a non-buggy one 😅

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 20, 2023

Thanks for the review @AnssiAhola and nice catch: Could really be a bug in that linter with the bare-url.

So, to conclude all this please @lemonase remove the < > brackets from links and leave the empty lines between the chapters.

Thanks again!

@lemonase
Copy link
Contributor Author

Thanks you guys, I've reverted any type of formatting and just left the code examples.

But I did figure out the reason for the angle brackets...
All the links in README.mkd begin on newlines, which makes markdownlint think they are bare links.
When the text and link are on the same line, no angle brackets.

@@ -11,7 +11,7 @@ It also supports OAuth 1.0a authorization, which allows you to change user data
such as profile information, collections and wantlists, inventory, and orders.

Find usage information [on our documentation pages](
<https://python3-discogs-client.readthedocs.org>),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

markdownlint thought this was a "bare link", hence angle brackets.

@prcutler
Copy link
Contributor

Sorry for the delay - LGTM! And thanks for the PR!

@JOJ0
Copy link
Contributor

JOJ0 commented Jan 21, 2023

anyone give me a approve button push. thanks :-)

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!

Copy link
Contributor Author

@lemonase lemonase left a comment

Choose a reason for hiding this comment

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

Comment for final review

@lemonase lemonase requested review from JOJ0 and removed request for prcutler and alifhughes January 22, 2023 04:02
Copy link
Contributor

@JOJ0 JOJ0 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your patience, I couldnt find the right buttons in the mobile github app ;-). Improving docs is always very welcome snd helps the project and most of all future users! all the best!

@JOJ0 JOJ0 merged commit 6292146 into joalla:master Jan 22, 2023
@lemonase lemonase deleted the docs_user_inventory branch January 26, 2023 03:22
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

4 participants