search

turnage / graw

Golang Reddit API Wrapper
MIT License
288 stars 49 forks source link

implementing autoRefresh of access token #53

Closed monkeydioude closed 4 years ago

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

monkeydioude commented 4 years ago

I will definitely add tests and make sure the existing ones pass. I prefer for you to have a first look at it, so I don't have to also change tests in case the commited code must change.

turnage commented 4 years ago

I will try to explain the reason I want the other changes removed.

To me the point of isolating commits to a repo once it has dependencies is for bisection.

When something breaks, I want to be able to find the offending commit and revert it ASAP. Ideally, that can happen without needing to revert every following commit, and without merge conflicts.

Small extra deltas such as adding a newline to a file don’t seem like they matter, but they do when they mean the difference between a revert button I can click on my phone and having to go to a workstation to manually resolve the merge conflict.

So things like having a refactor that moves the base client logic into app client may be fine, but they should be done separately, because if I have to revert this commit, but a later commit depends on that refactor, it becomes complicated and manual to resolve.

In the end I want commits as small and revert-able possible. I would be happy to look at separate PRs for your other changes if you would still like to make them.

monkeydioude commented 4 years ago

I understand. I can split in commits so each controversial part is in its own commit.

monkeydioude commented 4 years ago

Except the non existing tail call optimization (thanks for that btw, I did not know and I have some for to make in some of my repos 🙃), I don't agree. You made it clear arguing further was pointless. You're the package owner, I'll close this PR and make it synchronously, so people can still use their bots after 1hour.