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

User repos caching improvements #18

Merged
merged 3 commits into from
Jan 20, 2020
Merged

User repos caching improvements #18

merged 3 commits into from
Jan 20, 2020

Conversation

fahrenq
Copy link
Contributor

@fahrenq fahrenq commented Jan 13, 2020

Hello again, @edgarjs!

Added improvements mentioned in the previous PR:

  • Replaced ReposFormatter with Repo class, which is capable of the same thing, except that it's doing it per repository and abstracts repository to just @name and @link. This improved performance a lot.
  • Implementing the above functionality required extending local_storage class to allow saving raw strings.
  • Since we're mostly don't do any IO on gh <term> command now, I disabled Alfred's "delay after type" functionality.
  • Added cache expiration.
  • Updated Readme according to all recent changes.

Possible improvements:

  • github/api.rb becomes a little bit too big and not just an API connector anymore. Potentially, extracting all logic apart from actual API requests (along with all these @host stuff) would help for code quality.

Please review the changes carefully. Ruby is so magical and explicit, I easily could mess up something.

Thank you.

- RepoFormatter replaced with Repo class.
- Deleted delay after type in main gh alfred trigger.
- Local Storage class extended with functionality of saving and reading raw string instead of always serializing to JSON.
@fahrenq
Copy link
Contributor Author

fahrenq commented Jan 13, 2020

P.S. It should be compatible with the previous version since the cache will be considered outdated due to the lack of a "cache-timestamp" file.

Copy link
Owner

@edgarjs edgarjs left a comment

Choose a reason for hiding this comment

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

These are great changes, thank you. I left a few comments but feel free to leave them for later.

lib/github/api.rb Outdated Show resolved Hide resolved
lib/github/api.rb Outdated Show resolved Hide resolved
@fahrenq
Copy link
Contributor Author

fahrenq commented Jan 16, 2020

Hey @edgarjs,

Thanks for the review. I made some updates, please check :)

@fahrenq fahrenq requested a review from edgarjs January 16, 2020 16:30
Copy link
Owner

@edgarjs edgarjs left a comment

Choose a reason for hiding this comment

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

👍

@edgarjs edgarjs merged commit f954aad into edgarjs:master Jan 20, 2020
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