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

Add a user agent #1

Merged
merged 2 commits into from
Nov 26, 2020
Merged

Add a user agent #1

merged 2 commits into from
Nov 26, 2020

Conversation

stchris
Copy link
Contributor

@stchris stchris commented Nov 26, 2020

Hallo 👋

I'm loving this project. While trying to set up some reddit feeds, I stumbled across a request limit very fast and it seems like adding a user agent is a good thing anyway.

@fgeller
Copy link
Owner

fgeller commented Nov 26, 2020

@stchris !! Good to hear from you! And even better that the tiny app is helping you 😁

Couple of questions from my side:

  • Did you get around the request limits when adding the user agent? In either case I agree it makes sense to add it, just curious!

  • The test seems to be about unmarshaling the reddit feed, not related to the user agent correct? Again, just trying to make sure I understand - would gladly add the test!

Would gladly pull it in as is, the only tiny change I think I could recommend I added as a comment. What do you think? should I just merge the PR and move the line or do you feel like moving it? 😉

main.go Outdated
@@ -218,7 +221,13 @@ type AtomEntry struct {

func downloadFeed(url string) ([]byte, error) {
log.Printf("downloading feed %#v\n", url)
resp, err := http.Get(url)
client := http.Client{}
Copy link
Owner

Choose a reason for hiding this comment

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

not sure what the vim incantation is, but i'd move this closer to it's first use via client.Do -- say m5 or something should do it in vim? 😜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea. Done!

(I used a hex editor, vim is too high level for me)

Copy link
Owner

Choose a reason for hiding this comment

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

I assume that's hexl-mode? 😝

Copy link
Owner

Choose a reason for hiding this comment

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

But I think i'm missing the actual move?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🦅 👁️ ... commit without add will do that 🤦

Copy link
Owner

Choose a reason for hiding this comment

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

😉 cheers!

@stchris
Copy link
Contributor Author

stchris commented Nov 26, 2020

* Did you get around the request limits when adding the user agent? In either case I agree it makes sense to add it, just curious!

Yes, that helped. I suppose there are still some rate limits and there's no way you can use OAuth in an rss reader like they describe in https://github.com/reddit-archive/reddit/wiki/API.

* The test seems to be about unmarshaling the reddit feed, not related to the user agent correct? Again, just trying to make sure I understand - would gladly add the test!

Yes, the test seems out of context because I first suspected some parsing error so I started writing a basic test. I'm happy to split up the PR into multiple ones (or refactor the commits).

Would gladly pull it in as is, the only tiny change I think I could recommend I added as a comment. What do you think? should I just merge the PR and move the line or do you feel like moving it? wink

I like to move it, move it ...

@stchris stchris requested a review from fgeller November 26, 2020 20:42
@fgeller
Copy link
Owner

fgeller commented Nov 26, 2020

Yes, that helped. I suppose there are still some rate limits and there's no way you can use OAuth in an rss reader like they describe in https://github.com/reddit-archive/reddit/wiki/API.

Agreed, OAuth is a bit out of scope at this point.

Yes, the test seems out of context because I first suspected some parsing error so I started writing a basic test. I'm happy to split up the PR into multiple ones (or refactor the commits).

Nah, I don't mind - if you feel like it, sure, but I'd pull the changes in as is as well 👍

I like to move it, move it ...

I'd like to see the way you move.....

@fgeller fgeller merged commit 178f749 into fgeller:master Nov 26, 2020
@fgeller
Copy link
Owner

fgeller commented Nov 26, 2020

🙏 thanks a lot, again @stchris !

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

2 participants