Warn the user if they are not providing an auth token #34

biskit1 · 2018-11-28T02:04:53Z

Fixes issue #13 to warn user if they are not providing an auth token.
I know the code can probably be written more efficiently and be modularized- please share your thoughts and suggestions/ provide direction.

Some side notes:

Is there a way to retrieve the next page without making an entirely new api request? Given that each page counts for one 'request', if someone has a lot of stars (like @seanprashad) running the program more than once in the same hour (without auth token) uses up all the requests. The current behavior fails to display any stars on the second program call.
For eg: username seanprashad has 909 stars, and it's 30 per page. That's 31 requests for one program call. When I run the program a second time with same username, it fails to display any of the stars, even though it can technically make 29 more requests. @0xazure can you please point out why/where this behavior occurs in the code, and let me know if I should fix it/open another issue for this? Not sure if the original code had this behavior, or if my new code introduced this 'bug'. I can try to do some more testing later.
Not sure if this is relevant, but should we look into this for cacheing results to avoid wasting rate limit counts? Not sure how it would work between each program run, but let me know if you think it's something worth exploring and testing out.

seanprashad · 2018-11-28T02:12:31Z

Looks like Clippy, the linter we use, is exploding in Travis: https://travis-ci.org/0xazure/supernova/jobs/460568968#L657

0xazure

Hey @biskit1 this is awesome to see, thanks so much for working on this! 🎉

I've left a few inline comments, but overall this looks really great, and in my testing I've been very happy with the results. Your error messages in particular are phenomenal and communicate a lot of important and actionable information back to the user, so super-kudos for that!

As for your notes:

No, unfortunately each page request takes an API call. I've filed Investigate GraphQL #31 to investigate GraphQL to see if we can cut down on our requests and do away with the pagination, but for now we're stuck with the rate limiting. That's why Warn the user if they are not providing an auth token #13 is so important, so we can tell the user what's going on if they don' know anything about the GitHub API or why their requests start failing.
I don't actually think there's a good solution to this, because it's probably not desirable to only get some of the user's stars even if we tell the user we weren't able to finish all of the requests. Definitely log it as an issue though, we can revisit it.
Conditional Requests is an amazing find! Please file and issue for this, it would certainly save us a lot in terms of rate-limiting and looks like exactly what we should be using.

0xazure · 2018-11-28T03:17:50Z

src/lib.rs

+
+    let mut remaining: i32 = 0; 
+    let mut total: i32 = 0;
+    let mut timestamp_str = String::new();


Suggested change

let mut timestamp_str = String::new();

let mut timestamp_str = String::new();

nit: add a bit of whitespace between this statement and the following while loop.

0xazure · 2018-11-28T03:29:01Z

src/lib.rs


+            for header in res.headers().iter() {


I see you're iterating the map of Header values here. Is this because reqwest doesn't implement headers for the X-RateLimit- headers GitHub is returning to us?

If you're up for it/interested, we could implement the Header trait for these X-RateLimit- headers so we can get() the individual Headers based on their type. If not I totally understand, it's something we can turn into an issue for later improvement.

Yes, that's right. I've opened up issue #36 to implement the Header trait, and will change this code around once that is completed. Didn't realize I can do that, so thanks for the suggestion and link.

It doesn't necessarily have to be part of this pull request, the current approach works just fine.

I'd actually encourage that we leave #36 as a later enhancement and finish getting this pull request merged in, since implementing custom Headers isn't critical to getting these warnings implemented.

0xazure · 2018-11-28T03:29:44Z

src/lib.rs


+            for header in res.headers().iter() {
+                if header.name() == total_rate_limit {


If we're going to keep the map iteration, these if statements could probably be replaced with a match statement on header.name().

0xazure · 2018-11-28T03:35:36Z

src/lib.rs


+            for header in res.headers().iter() {
+                if header.name() == total_rate_limit {
+                    total = header.value_string().parse::<i32>().unwrap();


unwrap() is generally a dangerous function because it will panic and crash the program with user-unfriendly output (you can see an example of the unfriendliness in this Rust playground)

Instead, we should communicate a failure back to main() by returning the Resulting Error from the current function. The structure in supernova follows almost exactly the example CLI tool from Chapter 12.3 of the Rust Book (2nd Ed.), which shows how to refactor a panic!-based error approach into printing user-friendly errors, so take a look and see how the example in the Book matches up with the current implementation.

I'd recommend having a look at the try! macro and the ? operator for different ways to deal with errors in Rust.

0xazure · 2018-11-28T03:37:26Z

src/lib.rs

@@ -125,6 +167,11 @@ pub fn collect_stars(config: Config) -> Result<(), Box<dyn error::Error>> {
    }
    println!("Collected {} stars", stars.len());

+    match remaining {
+        10 | 0...5 => println!("Warning: You have {} out of {} requests remaining. Your request limit will reset at {}", remaining, total, timestamp_str),


This is awesome!!! The though you put into warning the user at 10 and then again at each increment from 5 down to 0 is really great to see, this will be very useful for the user to know exactly how many requests they have left at various usage points.

0xazure · 2018-11-28T03:44:25Z

src/lib.rs

 use reqwest::header::{qitem, Accept, Authorization, Bearer, Link, RelationType, UserAgent};
 use serde_derive::Deserialize;
 use std::{error, fmt, mem};
+use std::time::{UNIX_EPOCH, Duration};
+use reqwest::StatusCode;


nit: use statements in Rust are generally sorted in alphabetical order, so this should be with the other reqwest use statements. Running rustfmt will alert you to issues like this, though I realized it's not currently part of our continuous integration pipeline.

0xazure · 2018-11-28T03:54:27Z

src/lib.rs

@@ -103,18 +105,58 @@ pub fn collect_stars(config: Config) -> Result<(), Box<dyn error::Error>> {
    if let Some(ref token) = config.token {
        builder.set_authorization_token(token.to_owned());
    }
+    else {
+        println!("Authentication Warning: This is an unauthenticated request with a limit of 60 requests per hour. Re-run this program using an auth token by adding `--token <auth-token>` for an increased quota of 5000 requests per hour.")


This message is really great and descriptive! It's just a bit long and overruns my terminal width. Can we add some linebreaks in to improve the formatting and make it more readable?

We can probably drop the "Authentication Warning" part and re-word the beginning a bit.

It also gets a bit lost in the output because it happens before all of the stars are output, maybe this should be moved towards the bottom? Something like "Request completed without authentication, re-run using --token [...]"

Lastly, let's output this to stderr using eprintln! to better communicate that there is a problem the user should fix, and so it will still show up in the terminal if the user redirects stdout to a file.

0xazure · 2018-11-28T03:58:33Z

src/lib.rs

+                        let timestamp = reset_time.parse::<u64>().unwrap();
+
+                        // Creates a new SystemTime from the specified number of whole seconds
+                        let d = UNIX_EPOCH + Duration::from_secs(timestamp);


My understanding from GitHub's API docs was that X-RateLimit-Reset is already a value in UTC epoch seconds. Couldn't we create a chrono::DateTime directly from this value?

0xazure · 2018-11-28T04:01:01Z

src/lib.rs

@@ -125,6 +167,11 @@ pub fn collect_stars(config: Config) -> Result<(), Box<dyn error::Error>> {
    }
    println!("Collected {} stars", stars.len());

+    match remaining {
+        10 | 0...5 => println!("Warning: You have {} out of {} requests remaining. Your request limit will reset at {}", remaining, total, timestamp_str),


As I mentioned in another comment, let's output this to stderr using eprintln! to better communicate that there is a problem the user should fix.

0xazure · 2018-11-28T04:05:08Z

src/lib.rs

+                        let datetime = DateTime::<Local>::from(d);
+
+                        // Formats the combined date and time with the specified format string.
+                        timestamp_str = datetime.format("%Y-%m-%d %I:%M").to_string();


Not totally sold on the date format here. In my test I ended up with 2018-11-27 11:13. %I gives you the 12-hour hour number, but without AM or PM it's not very useful.

Since the request limit generally resets every hour, I think it would be more useful to tell the user a number of minutes (or hours, potentially) until their limit resets, rather than a full date, e.g. "Your request limit will reset in 24 minutes".

In reading about DateTime::format it seems it returns a DelayedFormat object that "can be used as an argument to format! or others", so it doesn't need to be converted using to_string and can be passed to format! directly.

0xazure · 2018-11-28T04:17:15Z

src/lib.rs

+
+    let mut remaining: i32 = 0; 
+    let mut total: i32 = 0;
+    let mut timestamp_str = String::new();


I don't think timestamp_str needs to be initialized here, and actually costs us an allocation because of String::new().

seanprashad · 2018-11-29T02:41:51Z

More errors 😓https://travis-ci.org/0xazure/supernova/jobs/461088364#L660

biskit1 · 2018-11-29T14:41:33Z

@seanprashad I think it's failing more because I pushed a commit, so it ran again...
@0xazure thanks for your comprehensive review. I will go through them ASAP, and have opened issue #35 to look into conditional requests.

biskit1 · 2018-12-18T19:05:23Z

Alrighty @0xazure, finally got to making those changes. Let me know what you think... Did something a bit different for the time updates. It's working, but weird. Couldn't figure out any other way to get it to do that, so if you have a different better suggestion please let me know.
Note: as it stands, when the current time reaches the reset time, the message will display "Your request limit will reset in 0 minutes." for that entire minute until it all resets.

0xazure

Hey @biskit1 this is looking really great, we're almost there!

A few more comments for you, and I'll give this code a spin soon to see if I have any comments about functionality or error messages like I did last time.

0xazure · 2018-12-19T04:08:14Z

src/lib.rs

 #[derive(Debug)]
 pub struct Config {
    username: String,
    token: Option<String>,
 }

 impl Config {
-    fn url(self) -> Option<String> {
+    fn url(self) -> Option<String> { 


Suggested change

fn url(self) -> Option<String> {

fn url(self) -> Option<String> {

nit: trailing whitespace

0xazure · 2018-12-19T04:12:47Z

src/lib.rs

 use std::{error, fmt, mem};

+
+


nit: extra vertical whitespace

0xazure · 2018-12-19T04:19:00Z

src/lib.rs

+                        let seconds = header.value_string().parse::<u64>()?;
+
+                        // Creates a new SystemTime from the specified number of whole seconds
+                        reset_time = UNIX_EPOCH + Duration::from_secs(seconds);


I think this got lost in the last review, so I'm reposting it:

My understanding from GitHub's API docs was that X-RateLimit-Reset is already a value in UTC epoch seconds. Couldn't we create a chrono::DateTime directly from this value?

@0xazure Creating a date from the seconds is pretty straight forward (if we are displaying the time at which rate limit resets). Tried lots of stuff to try and obtain minutes remaining though, and can't seem to come up with something just using chrono::DateTime... Want to take a stab at it?

0xazure · 2018-12-19T04:27:48Z

src/lib.rs

+            match res.status() {
+                StatusCode::Forbidden => {
+                    //this type of err, or panic? 
+                    return Err(format!("Uh-oh! You have {} out of {} requests remaining. Your request limit will reset in {} minutes.", remaining, total, reset_time.duration_since(SystemTime::now())?.as_secs()/60).into());


Not thrilled with the magic number 60 here (and elsewhere).

If we switched over to using chrono types (see the use chrono line, we're already using this library elsewhere) for all of the datetime work, we could use the strftime format syntax which I think would be a lot clearer.

see my comment here

0xazure · 2018-12-19T04:30:03Z

src/lib.rs

+
+    match remaining {
+        10 | 0...5 => eprintln!("Warning: You have {} out of {} requests remaining. Your request limit will reset in {} minutes.", remaining, total, reset_time.duration_since(SystemTime::now())?.as_secs()/60),
+        _ => println!("Warning: You have {} out of {} requests remaining. Your request limit will reset in {} minutes.", remaining, total, reset_time.duration_since(SystemTime::now())?.as_secs()/60),  


Is this intentional? In a previous commit you had

_ => (),

but how it is now will print the warning message (to stdout) every time, regardless of the value of remaining.

Woops! Put that in for testing and forgot to remove.

0xazure · 2018-12-19T04:31:26Z

src/lib.rs

+
+            match res.status() {
+                StatusCode::Forbidden => {
+                    //this type of err, or panic? 


Suggested change

//this type of err, or panic?

nit: extra comment

0xazure

Looks like clippy is warning about match vs if let:

error: you seem to be trying to use match for destructuring a single pattern. Consider using `if let`
   --> src/lib.rs:146:13
    |
146 | /             match res.status() {
147 | |                 StatusCode::Forbidden => {
148 | |                     return Err(format!("Uh-oh! You have {} out of {} requests remaining. Your request limit will reset in {} minutes.", remaining, total, mins).into());
149 | |                 },
150 | |                 _ => (),
151 | |             }
    | |_____________^
    |
    = note: `-D clippy::single-match` implied by `-D warnings`
    = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/master/index.html#single_match

Also, @biskit1 you said

Creating a date from the seconds is pretty straight forward (if we are displaying the time at which rate limit resets). Tried lots of stuff to try and obtain minutes remaining though, and can't seem to come up with something just using chrono::DateTime... Want to take a stab at it?

In another comment I suggested looking into chrono::DateTime::format and the strftime format syntax to handle formatting instead of doing the math ourselves. You're right, chrono::DateTime doesn't have a method that gives the minutes remaining because the preferred (and much more flexible) way to get this information for display is the chrono::DateTime::format method.

The chrono crate documentation has a lot of examples and might be worth having a look at, particularly the sections on formatting and parsing and conversions from and to EPOCH timestamps.

biskit1 · 2018-12-28T22:06:42Z

Thanks for the review @0xazure, and for the further direction :)
For some reason date-time related things freak me out, but I'll read through what you linked and try to get a better handle on it before making the change. Hopefully we can merge this before we update the entire codebase ;) lol

0xazure · 2018-12-28T22:18:28Z

Date-time stuff is pretty challenging to get right, so that's why I'm trying to get us to use more library functions for this instead of doing it ourselves. None of what you've done so far is incorrect or wrong, I'd just like us to use other people's code for this as much as possible; there are of course trade-off associated with using the chrono stuff (like all the reading, sorry about that 😞) but it'll be worth it 💪.

I also really appreciate your dedication to getting this feature-complete! Looking forward to getting it merged in.

biskit1 added 4 commits November 24, 2018 21:23

add missing auth token warning

14783fb

add rate limit warnings

4bda7c4

change wording of warning message

78a8350

added some error checking code

e3b6bd9

0xazure requested changes Nov 28, 2018

View reviewed changes

0xazure reviewed Nov 28, 2018

View reviewed changes

change match s=> to _=>

ba7c869

biskit1 mentioned this pull request Nov 29, 2018

Look into and test conditional request implementation #35

Open

biskit1 mentioned this pull request Nov 29, 2018

Implement Header traits for X-RateLimit headers #36

Open

seanprashad mentioned this pull request Dec 17, 2018

Investigate GraphQL #31

Open

made requested changes from PR review

489baae

biskit1 force-pushed the iss-13 branch from 1923d26 to 489baae Compare December 18, 2018 19:08

0xazure requested changes Dec 19, 2018

View reviewed changes

fixed nit reviews

13463f3

0xazure requested changes Dec 28, 2018

View reviewed changes

0xazure mentioned this pull request Dec 28, 2018

Ensure Travis fails fast #46

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn the user if they are not providing an auth token #34

Warn the user if they are not providing an auth token #34

biskit1 commented Nov 28, 2018 •

edited

seanprashad commented Nov 28, 2018

0xazure left a comment

0xazure Nov 28, 2018

0xazure Nov 28, 2018

biskit1 Nov 29, 2018 •

edited

0xazure Nov 30, 2018

0xazure Nov 28, 2018

0xazure Nov 28, 2018

0xazure Nov 28, 2018

0xazure Nov 28, 2018

0xazure Nov 28, 2018

0xazure Nov 28, 2018 •

edited

0xazure Nov 28, 2018 •

edited

0xazure Nov 28, 2018 •

edited

0xazure Nov 28, 2018

seanprashad commented Nov 29, 2018

biskit1 commented Nov 29, 2018

biskit1 commented Dec 18, 2018 •

edited

0xazure left a comment

0xazure Dec 19, 2018

0xazure Dec 19, 2018

0xazure Dec 19, 2018

biskit1 Dec 19, 2018 •

edited

0xazure Dec 19, 2018

biskit1 Dec 19, 2018 •

edited

0xazure Dec 19, 2018

biskit1 Dec 19, 2018

0xazure Dec 19, 2018

0xazure left a comment

biskit1 commented Dec 28, 2018

0xazure commented Dec 28, 2018

	let mut timestamp_str = String::new();
	let mut timestamp_str = String::new();


		for header in res.headers().iter() {
		if header.name() == total_rate_limit {

	fn url(self) -> Option<String> {
	fn url(self) -> Option<String> {

Warn the user if they are not providing an auth token #34

Are you sure you want to change the base?

Warn the user if they are not providing an auth token #34

Conversation

biskit1 commented Nov 28, 2018 • edited

seanprashad commented Nov 28, 2018

0xazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biskit1 Nov 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xazure Nov 28, 2018 • edited

Choose a reason for hiding this comment

0xazure Nov 28, 2018 • edited

Choose a reason for hiding this comment

0xazure Nov 28, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanprashad commented Nov 29, 2018

biskit1 commented Nov 29, 2018

biskit1 commented Dec 18, 2018 • edited

0xazure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biskit1 Dec 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

biskit1 Dec 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xazure left a comment

Choose a reason for hiding this comment

biskit1 commented Dec 28, 2018

0xazure commented Dec 28, 2018

biskit1 commented Nov 28, 2018 •

edited

biskit1 Nov 29, 2018 •

edited

0xazure Nov 28, 2018 •

edited

0xazure Nov 28, 2018 •

edited

0xazure Nov 28, 2018 •

edited

biskit1 commented Dec 18, 2018 •

edited

biskit1 Dec 19, 2018 •

edited

biskit1 Dec 19, 2018 •

edited