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

reduce repeated examples. #113

Merged
merged 54 commits into from
Aug 27, 2020
Merged

reduce repeated examples. #113

merged 54 commits into from
Aug 27, 2020

Conversation

ramsayleung
Copy link
Owner

@ramsayleung ramsayleung commented Aug 16, 2020

All of these example files come from a same template, which are are quite repetitive and don't offer much help for real projects, the only difference between them is that they calls different function with different day, everything else is same.

  • remove repetitive examples.
  • add some real examples.
  • update document.

@ramsayleung ramsayleung mentioned this pull request Aug 16, 2020
4 tasks
@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 16, 2020

I would reuse the examples and turn them into tests, rather than completely remove them. That way we have full coverage for every endpoint. Oh I've seen these tests already exist.

I wanted to suggest to use a refresh token with all scopes already given for the test_with_oauth. We could use it to avoid user interaction, and that way they can be ran with Travis as well, since they aren't supposed to expire. This requires a premium account for the library, so I made a request for an account to Spotify - https://community.spotify.com/t5/Spotify-for-Developers/Dummy-premium-accounts-for-library-developers/td-p/5018210

By the way, sorry about the force pushes, the 4 following commits don't change anything. The fifth adds a new example called with_refresh_token. Let me know what you think about it.

@ramsayleung
Copy link
Owner Author

I wanted to suggest to use a refresh token with all scopes already given for the test_with_oauth. We could use it to avoid user interaction, and that way they can be ran with Travis as well, since they aren't supposed to expire.

Yep, it's a great idea to run tests, and the key point how to get a non-expire refresh-token. Hopefully you could get the premium account.

By the way, sorry about the force pushes, the 4 following commits don't change anything. The fifth adds a new example called with_refresh_token. Let me know what you think about it.

The example called with_refresh_token is helpful to demonstrate how to refresh token, but I think force push it's not a good idea, perhaps it will overwrite something important.

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 17, 2020

No, the force push was because I created a commit by mistake and then dropped it later. You can do a rebase and merge and I think it'll be ok, without overwriting anything.

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 21, 2020

Ok so I think I've managed to get the GitHub README to be much more straight to the point. The documentation for Rspotify itself is now inside the docs.rs site instead of split between different places, and the GitHub README is now limited to basic information. Do let me know what you think about the changes I've made.

I suggest using cargo doc --features=blocking to have access to these docs as well. Not sure if this can be included in Cargo.toml.

@ramsayleung
Copy link
Owner Author

ramsayleung commented Aug 22, 2020

The documentation for Rspotify itself is now inside the docs.rs site instead of split between different places,

Sorry for not getting your point, what do you mean about splitting between different places?

and the GitHub README is now limited to basic information.

I think it's the de-facto way to maintain documentation in docs.rs in Rust community, and leading the people to the documentation in docs.rs by putting a link in the Github README, is it meaning of limitation in your opinion? I think it's a acceptable way to maintain documentation.IMHO, the key point of documentation is its quality, not the place keeps it.

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 22, 2020

Sorry for not getting your point, what do you mean about splitting between different places?

What I mean is that, in my opinion, the README currently has too much documentation. It shows how to configure Rspotify (its features and such), how authorization works, and includes a couple examples. I think it's easier to have everything documentation-related in docs.rs and just have the README point to the site. That way we don't have repeated documentation either, because I realized that most of the README was copy-pasted into the docs.rs page anyway.

For example the "Getting Started" section shouldn't be in the README IMO because that's related to the documentation. And by having the examples in the documentation, they are also ran with cargo test to make sure they're valid.

I'm just trying to follow what tekore, rand, bitflags and other libraries do, since I consider the documentation easier to follow that way. Their READMEs only contain information regarding the development of the library, since GitHub is only supposed to serve as a platform for development and hosting. Thus, I just moved a few sections to docs.rs. If you consider any of them could fit better in the README, let me know.

@ramsayleung
Copy link
Owner Author

If you consider any of them could fit better in the README, let me know.

I get your point now, I think is the good way to follow the best practice, as what other libraries do, moving some section from Github to docs.rs.

@marioortizmanero
Copy link
Collaborator

Great! All that's left is adding more examples, but I'm not sure how many you want before this is merged. Do you plan on adding any yourself?

@ramsayleung
Copy link
Owner Author

ramsayleung commented Aug 23, 2020

Do you plan on adding any yourself?

Yep, I am planning to add some real-world examples.

PS: I am adding a new Web App example based on Rocket

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Aug 27, 2020

Neat example with the Rocket web app, I like it. Can this be merged for now, then? We can add more examples in the future and this is blocking other issues/PRs.

@ramsayleung
Copy link
Owner Author

Yep, merged :)

@ramsayleung ramsayleung merged commit 9fd5173 into master Aug 27, 2020
@ramsayleung ramsayleung deleted the reduce-examples branch August 27, 2020 14:29
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.

2 participants