-
Notifications
You must be signed in to change notification settings - Fork 60
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
Astropy partner page: Fix broken links #520
Conversation
@@ -32,7 +32,7 @@ core package for Astronomy in Python. Astropy also fosters an ecosystem of | |||
interoperable astronomy packages. Please remember to acknowledge and cite the use of any {{ page.community }} | |||
packages that you use. | |||
|
|||
Astropy currently has {{ total_packages }} packages that have been accepted by pyOpenSci and also become affiliated Astropy packages through our [partnership](partners.html). | |||
Astropy currently has {{ total_packages }} packages that have been accepted by pyOpenSci and also become affiliated Astropy packages through our [partnership](/partners.html). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix is copied from L12 above which is not a broken link in production but does not seem to want to render in PR preview: {"message":"Route Not Found"}
🤷♀️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pllim is it broken in circle CI? we do have some issues with relative links and images not rendering in our CI. I haven't had time to try to troubleshoot it but i think it relates to not having {{ base.url }} configured properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Configuring for base.url
is beyond me because the URL for each PR review is different. I think the preview bot needs to be smart enough to set this dynamically. Over as Astropy, we use RTD preview so we never have to worry about setting that manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I am pointing out is that the fix in this line cannot really be proven to work until after merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i hear you. i wasn't asking you to fix anything. we've just had ongoing issues with our preview not always working. So the best thing to do it to build locally.
when i build this pr locally, it all looks fine to me - the links work etc . Let's merge this. Then i opened 3 issues in the pyosmeta repo that highlight the issues with the data that we are seeing.
when we merge we can see if the astropy website works again as well.
what's hardest with our data generation here is that people always do slightly different things in issues. so here, the maintainers aren't listed with commas for stingray. we could fix that in the issue but i think we should also account for the other items that you fixed here over at pyosmeta.
The failure about Zenodo is unrelated. |
because I went down the rabbit hole trying to debug failing CI
@@ -46,10 +46,11 @@ <h3 class="card__title no_toc" itemprop="headline"> | |||
<li><a href="{{ apackage.archive }}" rel="permalink"><i class="fas fa-bookmark fa-fw"></i> JOSS Approved</a></li> | |||
{% endif %} | |||
{% if apackage.astropy %} | |||
<li><a href="partners/astropy.html"><i class="fa-solid fa-check-double"></i> <img src="http://img.shields.io/badge/Affiliated-Astropy-orange.svg?style=flat" alt="Astropy" /></a></li> | |||
<li><a href="communities/astropy.html"><i class="fa-solid fa-check-double"></i> <img src="http://img.shields.io/badge/Affiliated-Astropy-orange.svg?style=flat" alt="Astropy" /></a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This communities vs partners is confusing me. I think this got rid of one of the Jekyll errors though but I cannot be sure.
_data/packages.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some fields are plain wrong. Some are just inconsistent. I thought fixing this file would fix the Jekyll errors but nope. No idea where these are coming from. If you want me to revert some of these changes, please let me know.
### Errors in _site/index.html
Error: R] [file:///home/runner/work/pyopensci.github.io/pyopensci.github.io/_site/[![DOI](https:/zenodo.org/badge/DOI/10.5281/zenodo.10951577.svg)](https:/doi.org/10.5281/zenodo.10951577)](file:///home/runner/work/pyopensci.github.io/pyopensci.github.io/_site/[![DOI](https:/zenodo.org/badge/DOI/10.5281/zenodo.10951577.svg)](https:/doi.org/10.5281/zenodo.10951577)) | Failed: Cannot find file
### Errors in _site/python-packages.html
Error: R] [file:///home/runner/work/pyopensci.github.io/pyopensci.github.io/_site/[![DOI](https:/zenodo.org/badge/DOI/10.5281/zenodo.10951577.svg)](https:/doi.org/10.5281/zenodo.10951577)](file:///home/runner/work/pyopensci.github.io/pyopensci.github.io/_site/[![DOI](https:/zenodo.org/badge/DOI/10.5281/zenodo.10951577.svg)](https:/doi.org/10.5281/zenodo.10951577)) | Failed: Cannot find file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the wrong fields are choking our JS over at https://www.astropy.org/affiliated/#affiliated-package-list so would be nice if it gets fixed either in this PR or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's try to tease out this into separate issues. I do have someone who can help fix this that I just onboarded @pllim so let's focus on fixing the links in this pr. then let's open a new pr related to your affiliated package listing. i'm confused as to why that is broken because we haven't actually changed our data ingest process at all. (i think!) but clearly it is broken.
@@ -2,11 +2,17 @@ | |||
package_description: A spectral-timing software package for astrophysical X-ray | |||
(and other) data | |||
submitting_author: | |||
name: Name | |||
name: Matteo Bachetti |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok so I think we need a new issue about this, specifically in pyos meta. what i'm seeing here is our code isn't processing multiple maintainers correctly. We will need to debug pyosmeta to see why.
@pllim i'm going to merge this. let's see if it fixes astropy related issues. i've also opened:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I built this locally and it looks good.
nice @pllim it looks like your website is building fine now! https://www.astropy.org/affiliated/#affiliated-package-list however, it's missing a package (I think). I think we have 3 astropy packages. |
Yes. The dev console is complaining about indexing. Maybe the problem is in JS over at astropy this time (astropy/astropy.github.com#643). |
ok well please let me know if you find other data bugs on our end! i definitely will make sure the open issues are addressed to clean our yaml file up!! parsing issue text is not an easy task because.. humans... do things that you don't expect them to do 😆 and because I'm a human, I do the same thing, too 🙃 one day, we'll have a form with validation is my thought. |
I think at the very least, have the bot open a PR to update any YAML data going forward and make sure a human vets the diff before being merged into production. I'm guessing the bot has commiting without human intervention? Since this project probably isn't accepting huge amounts of packages a day, human vetting shouldn't take long. Though fixing any bugs as a result might incur extra overhead. |
@pllim the bot opens a pr and is vetted by a human. I think what I need to do is to let the team know what to look for in the prs. As recently, others have been merging them which is great for me but they need to know what to look for. I'll post in slack about this! |
Also would someone from astropy want to be on the web team who can merge these prs? |
Re: #520 (comment) Web team is currently @dhomeier @hamogu @eteq so will have to ask them. (Have you noticed repeating names yet? The joy of OSS... 😆 ) |
YAML supports schema, right? Perhaps some of the verification can be automated using a schema + CI. Might be better than human eyes. |
Occasional campaign to update the entire file to use consistent format is also nice if possible, especially if the schema changes. When I combed through it by eyes, I can kinda see things evolved but old fields stayed the old style, so overall style is a little inconsistent. FYI. |
Myself and I think @hamogu as well mainly became part of that for curating sites.json entries. Not sure if I could be of much help in fixing JS bugs or those other failures you mention. |
Fix #519