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

Abstracts fact fetching for Github sources #59

Merged
merged 1 commit into from
Oct 23, 2016

Conversation

brendo
Copy link
Contributor

@brendo brendo commented Oct 23, 2016

This builds on #58, by introducing a GithubSourceFacts class which will use the Github API to retrieve the information. This is largely a refactor of existing code, with the additional of implementing sessionStorage caching.

@brendo
Copy link
Contributor Author

brendo commented Oct 23, 2016

@squidfunk Sorry, this seems to have a couple more files than what I was expecting. What flags are you using with gulp prior to commit?

I noticed release fetching in your latest commit, and the start of abstraction. If you're happy with the approach in this PR I could roll that in as well. Up to you!

@squidfunk
Copy link
Owner

You should run the build before the commit. Only the scripts in package.json should be used:

npm run build

I will rework the documentation to reflect that and add a CONTRIBUTING.md However, as everything's in progress, it's doesn't really matter.

I'm sorry for all the mess in application.js, I'm still in prototyping-mode. Everything will be cleaned up or the release. Thanks for abstracting this stuff.

@squidfunk squidfunk merged commit 60f48d7 into squidfunk:rework Oct 23, 2016
@squidfunk
Copy link
Owner

BTW: good idea on using the session storage for the repository facts - was on my internal list. Did I mention it somewhere here or was that your own idea?

@brendo
Copy link
Contributor Author

brendo commented Oct 23, 2016

That was your idea, I stumbled across the to-do while abstracting the logic. Thanks for the merge!

@brendo brendo deleted the github-source-facts branch October 23, 2016 23:29
@squidfunk
Copy link
Owner

Awesome! Thanks for going ahead and doing it.

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