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 NumPy Team #205

Merged
merged 11 commits into from
Apr 16, 2020
Merged

Add a NumPy Team #205

merged 11 commits into from
Apr 16, 2020

Conversation

shaloo
Copy link
Contributor

@shaloo shaloo commented Mar 26, 2020

NumPy Team gallery added, script to generate gallery added.
Fixes gh-45

Deployed URL

http://numpy-205.surge.sh/

@shaloo shaloo requested a review from rgommers March 26, 2020 17:00
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

Thanks @shaloo, the output looks pretty good!

Code wise there's some cleanups to do to make this easier to maintain. A lot of committed files are unneccesary. I propose to aim for the following structure:

  • scripts/gallery/numpy_team.py
  • scripts/gallery/numpy_team.yml
  • scripts/gallery/cache/numpy_team.context
  • content/en/team.md
  • layouts/partials/team_gallery.html

As far as I can tell, that's all you need. And then the page would end up at /team rather than /gallery/team

scripts/gallery/numpy/static/css/styles.css Outdated Show resolved Hide resolved
scripts/gallery/numpy/team.md Outdated Show resolved Hide resolved
scripts/gallery/team.py Outdated Show resolved Hide resolved
scripts/gallery/team.py Outdated Show resolved Hide resolved
scripts/gallery/team.py Outdated Show resolved Hide resolved
@shaloo
Copy link
Contributor Author

shaloo commented Mar 29, 2020

Thanks @shaloo, the output looks pretty good!

Code wise there's some cleanups to do to make this easier to maintain. A lot of committed files are unneccesary. I propose to aim for the following structure:

  • scripts/gallery/numpy_team.py
  • scripts/gallery/numpy_team.yml
  • scripts/gallery/cache/numpy_team.context
  • content/en/team.md
  • layouts/partials/team_gallery.html

As far as I can tell, that's all you need. And then the page would end up at /team rather than /gallery/team

What about _templates? Those are used by Jinja2 to spit out html (static html with no variables unlike partials). Or are you suggesting we use partial/team.html as the template for Jinja2 translations?

scripts/gallery/team.py Outdated Show resolved Hide resolved
@rgommers
Copy link
Member

What about _templates? Those are used by Jinja2 to spit out html (static html with no variables unlike partials). Or are you suggesting we use partial/team.html as the template for Jinja2 translations?

As I commented on #45 (comment), I missed the jinja2 bit. I think Jinja2 usage is undesirable here, since the little it does is something Hugo can do just as well. However, we can leave that for later in a separate issue, given that now it works.

The duplication of the CSS and images would still be good to fix though, that really does look unnecessary and it will go out of sync as soon as the other styles.css is changed (e.g. @joelachance is tweaking colors right now).

@shaloo
Copy link
Contributor Author

shaloo commented Mar 30, 2020

Thanks @shaloo, the output looks pretty good!

Code wise there's some cleanups to do to make this easier to maintain. A lot of committed files are unneccesary. I propose to aim for the following structure:

  • scripts/gallery/numpy_team.py
  • scripts/gallery/numpy_team.yml
  • scripts/gallery/cache/numpy_team.context
  • content/en/team.md
  • layouts/partials/team_gallery.html

As far as I can tell, that's all you need. And then the page would end up at /team rather than /gallery/team

Done. At present, master seems to have some issues - the build shows empty page so I have not been able to validate whether modifying team.md in content/en/ folder with the one I am using (that has Jinja directives) will play well with Hugo. Once the base master builds successfully without the changes I have made in scripts/gallery area, I will issue a PR. The script reorg as suggested above minus team.md relocation to suggested dir has been validated - both with GitHub as well as cache/numpy_team.context data.

@shaloo shaloo requested a review from rgommers March 30, 2020 11:24
@shaloo shaloo self-assigned this Apr 9, 2020
@shaloo
Copy link
Contributor Author

shaloo commented Apr 9, 2020

Thanks @shaloo, the output looks pretty good!

Code wise there's some cleanups to do to make this easier to maintain. A lot of committed files are unneccesary. I propose to aim for the following structure:

  • scripts/gallery/numpy_team.py
  • scripts/gallery/numpy_team.yml
  • scripts/gallery/cache/numpy_team.context
  • content/en/team.md
  • layouts/partials/team_gallery.html

As far as I can tell, that's all you need. And then the page would end up at /team rather than /gallery/team

All these are addressed now other than last two bullets. Those will be part of #213 which will remove jinja2 dependency.

@rgommers rgommers changed the title Sfork issu 45 Add a NumPy Team Apr 14, 2020
@rgommers
Copy link
Member

I removed --ignore-errors, I'm finding that that easily drops people off the gallery silently.

Would be nice if there was an --update-cache mode that doesn't so easily hit the rate limit and fills in the gaps.

The current cache in this PR is incomplete, I'll see if GitHub lets me update it later today.

@rgommers
Copy link
Member

The rest of changes look good, my earlier comments were addressed.

@rgommers
Copy link
Member

One other thing gh-213 will fix is that the current page doesn't have a navbar. Not a blocker though. The gallery looks good, and the current cache is updated. The GitHub rate limiting behavior is such that a single run of this script is fine, but a couple in a row is not. If this grows a lot, we may have to add an incremental mode at some point.

Merging, thanks @shaloo!

@rgommers rgommers merged commit b7a17df into numpy:master Apr 16, 2020
@@ -0,0 +1,1059 @@
ignore_io_errors: false
Copy link
Member

Choose a reason for hiding this comment

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

Is this file supposed to be committed? The fact it dupicates the yaml makes me think that perhaps it shouldn't be.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, see the discussion above - it's the output of retrieving from the GitHub API, which we don't want to do all the time.

@rgommers
Copy link
Member

I deleted the travisci.org webhook, future PRs should only get a single trigger to travisci.com. Hopefully that fixes the confusing CI status.

@shaloo shaloo deleted the sfork-issu-45 branch April 20, 2020 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Idea #1 for Team page
3 participants