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

Wrong date used for last commit date #26

Closed
rgroves opened this issue Oct 15, 2019 · 6 comments
Closed

Wrong date used for last commit date #26

rgroves opened this issue Oct 15, 2019 · 6 comments

Comments

@rgroves
Copy link
Contributor

rgroves commented Oct 15, 2019

This was reported in a previous issue (#12) and was decided it was best to open a separate issue for it. Below are the original reported details:

...the value being used to report the last commit is the "pushed_at" key of the github repo object. This time value is representative of the time a commit was made to any branch of the repo, not just the master branch. So this is not reflective of recency of the master branch that users would probably be looking for. I think the correct value to use would be the date of the last commit of the master branch.

Using the repo hauke96/hugo-theme-hamburg as an example:

  • Retrieving the repo's object and looking at the pushed_at key we see (at the time of this writing): "2019-08-23T13:18:15Z"
  • Retrieving the master branch's last commit object and looking at the commit.author.date key we see (at the time of this writing): "2019-08-19T23:59:44Z"
  • Retrieving the dev branch's last commit object and looking at the commit.author.date key we see (at the time of this writing): "2019-08-23T13:18:14Z"

Given the above, the value used as the last commit ("2019-08-23T13:18:15Z") is really representative of the dev branch, not the master branch. If the last commit is supposed to representative of repo activity that might be fine, it depends on what is meant to be conveyed and may be worthy of clarification on the front-end (e.g. maybe "last activity" is better instead of "last commit" which I think might be thought of as "last commit on master").

JugglerX responded to the above via comment in the previous issue:

  • Get the correct value for last_commit from the Github API in the generate-github.js script. It should be the "last commit on master"
@rgroves
Copy link
Contributor Author

rgroves commented Oct 15, 2019

I'm working on modifying the generate-github.js script now.

One issue I'm running into is that if the branch is not set explicitly in the theme's YAML front matter then "master" is assumed as the default, but master is not always present in the repo (example) which then results in a 404 when making the call to the github API branches endpoint for getting the last commit date for the branch.

This can be dealt with in a couple of ways:

  1. Ignore the theme, report/log it, but still allow the data/themes.json file to be generated for the other themes
  2. Instead of assuming "master" when no branch is specified, use the default_branch property from the main repo object that was retrieved in the initial get repos call.

It is easy enough to go with option 2, but that would assume the default branch is the correct one to use, which seems like it might be reasonable though there is something to be said for having the branch set explicitly to the correct one in the front matter. What do you think?

@JugglerX
Copy link
Contributor

I think number 2 seems like the correct approach. I imagine most themes default_branch property is still set to master right?

@rgroves
Copy link
Contributor Author

rgroves commented Oct 16, 2019

Yes, for the most part the default_branch is master, however there were around 56 repos that did not have a master branch. I didn't go through them all but the handful I manually examined seemed to forgo the "master" branch in favor of a "gh-pages" branch as the default branch.

I'll continue making the modifications incorporating option 2 from above.

@rgroves
Copy link
Contributor Author

rgroves commented Oct 17, 2019

A quick update on this issue. I'm running into a few problems with this because throughout some of the templates there are checks for Params.github_branch which comes from the theme's front matter, which if not present is being defaulted to "master". That default won't be correct for the themes that do not have a master branch.

For instance, in index.html there is this...

    {{ $repoName := printf "%s-%s" (substr (replace .Params.github "/" "-") 19 | urlize) (cond (ne .Params.github_branch nil) .Params.github_branch "master") }}
    {{ $repo := index .Site.Data.themes $repoName }}

...and for a repo like github.com/Phlow/feeling-responsive where there is no master branch in the above $repo never gets set because the wrong $repoName is being constructed when the branch is defaulted to "master" since no github_branch is set in that theme's front matter.

The scrub script that you mentioned in your comment on issue #12 might be the key to solving this. In addition to the"stale" property that is to be added, if the github_branch is updated as well with the correct branch from data/themes.json (which as part of this change is now retrieved and set from the github api using the default_branch property of the repo) then the problem will resolve itself.

I believe the scrub script would need to be changed in how it's driving the processing—right now the main loop is being driven by the files present in the content/theme directory, but I think it will need to be driven by the keys in the object present in the data/themes.json file. The reason being the branch name is part of the themeKey that gets built and there's no way to reconstruct it properly from the front matter alone (since for themes without a branch name specified it cannot be defaulted to master).

I can take a crack at the scrub script but wanted to check with you first for, 1) your thoughts on the above and 2) to ensure you're not also working on the scrub script.

@JugglerX
Copy link
Contributor

Yes the scrub script can almost be completey re-written, it was not ready at all. I am not working on it.

Your right the scrub script needs its main loop to run off data/themes.json. Reading the branch name from data/themes.json is the way forward.

We still need to support people optionally declaring their branch name in the front-matter.

@rgroves
Copy link
Contributor Author

rgroves commented Nov 6, 2019

Looks like this was handled in commit 987e109 so have closed the issue.

Sorry, I really should have treated #42 and this as one issue/commit since they were very tightly related. I think I was confused due to my misunderstanding of where the stale property was going to reside.

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

No branches or pull requests

2 participants