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

Feat: add text associated with astropy partnership #225

Merged
merged 4 commits into from
Mar 1, 2024

Conversation

lwasser
Copy link
Member

@lwasser lwasser commented Jul 5, 2023

This is a draft of the landing page for the astropy partnership in our peer review guide. This page specifies the requirements for a package to become astropy-affiliated.

it is based on this document in which i took all of the affiliated requirements that i could find from the astropy website / github and tried to match them with our default requirements. i then pulled out the astropy specific elements. Any and all feedback is welcome on this pr.

NOTE: i'm going to also loop in the pyhc / sunpy / plasmapy folks on this - just so they can see what the process looks like.

cc: @dstansby @namurphy can you kindly tag anyone else who might want to see this process please? i am not yet connected with all of the communities here on github.


@lwasser
Copy link
Member Author

lwasser commented Jul 5, 2023

the failure is only because html proofer is looking for this page online and it doesn't exist - yet. otherwise all systems are a go! 🚀

@namurphy
Copy link

namurphy commented Jul 5, 2023

Potentially of interest to @jibarnum, @sapols, and @rebeccaringuette from PyHC!

@lwasser
Copy link
Member Author

lwasser commented Jul 5, 2023

Copy link

@namurphy namurphy left a comment

Choose a reason for hiding this comment

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

Had a few pretty minor suggestions. Overall this is sounding pretty great!

partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
Comment on lines 7 to 12
About Our Partnerships <self>
JOSS <joss>
Astropy <astropy>
Pangeo <pangeo>
Copy link

Choose a reason for hiding this comment

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

Great to see this list growing!

Copy link
Member Author

Choose a reason for hiding this comment

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

@namurphy i'm excited about it too!! we also just spoke with scverse!!

- Reviewed against current pyOpenSci guidelines and checks.
- Also reviewed against the Astropy-specific guidelines listed below.

(astropy-collaboration)=
Copy link

Choose a reason for hiding this comment

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

I didn't know you could do this in Markdown. Cool!

Copy link
Member Author

Choose a reason for hiding this comment

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

it might be a superpower of MyST . i really love MyST for all of our documentation and anything sphinx related. it's such a game changer in terms of flexibility / readability with simple syntax!

Copy link
Contributor

Choose a reason for hiding this comment

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

What is (astropy-collaboration)=? What should I be expected to see with this directive in the rendered page?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh @pllim that is a myst target. it's a nice way to have a consistent target link in case someone changes header text and forgets and also to just make a nice short target link. so you can think of it as a shortcut to make cross linking to section headers within a sphinx document less error prone. i'm not sure it's visible in the final rendered page however just internally it's helpful in case people change header names.

:::{figure-md} astropy-logo
:class: myclass

<img src="/images/astropy_project_logo.svg" alt="The astropy logo. The logo is an orange egg shape with a white snake spiraling in it. The text of the image says The Astropy Project on three lines." class="mb-1" width="400px">
Copy link

Choose a reason for hiding this comment

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

Yay for alt-text & image descriptions!

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying hard to ensure we have an accessible guide @namurphy !! i'm so glad you appreciate it!!!

@hamogu
Copy link

hamogu commented Jul 7, 2023

I think this captures Astropy's extra requirements well!

@pllim
Copy link
Contributor

pllim commented Jul 13, 2023

I have it in my notes that @eteq would very much want to review this but his response might be delayed.

@lwasser
Copy link
Member Author

lwasser commented Jul 13, 2023

@all-contributors please add @pllim for code, review

ok no problem @eteq - looking forward to your input here. in the meantime i'll add y'all as contributors to our guide here!!

@allcontributors
Copy link
Contributor

@lwasser

I've put up a pull request to add @pllim! 🎉

@lwasser
Copy link
Member Author

lwasser commented Jul 25, 2023

hi colleagues! i'm going to accept all of @namurphy edits given there hasn't been any feedback suggesting they are not quite right!! @eteq 👋 would love your feedback on this when you have time

@lwasser
Copy link
Member Author

lwasser commented Jul 25, 2023

this is failing because of hte same htmlproofer issue mentioned here - pyOpenSci/pyopensci.github.io#241 which will be fixed at some point in the future!

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

On a high level, this LGTM but I would like @WilliamJamieson and @dhomeier to fine tune the details, if necessary, because they are the current Editors from Astropy.

If you ever fixed the rendered HTML preview, I would love to see that before merge as well.

Thank you!

@lwasser
Copy link
Member Author

lwasser commented Aug 1, 2023

@pllim you can always view the rendered docs on circle ci here is this what you mean? it should update with every change to this pr as well so we can see what it looks like prior to merging. below is a screenshot from circle ci preview

Screen Shot 2023-08-01 at 9 28 09 AM

@lwasser
Copy link
Member Author

lwasser commented Aug 1, 2023

what i'd love to do in the future is work a bit on a nice graphic for our partnerships. when i do that i'll run ideas by everyone as well.

Copy link
Contributor

@pllim pllim left a comment

Choose a reason for hiding this comment

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

Minor comments, mostly with how we brand astropy (just the core library) vs Astropy as a project.

As for the integration rules, might have to fine tune when Astropy get to updating https://github.com/astropy/astropy-project/blob/main/affiliated/ contents but seems fine for now as written (i.e., does not conflict with what we already do over at Astropy but seems repetitive).

partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
- Reviewed against current pyOpenSci guidelines and checks.
- Also reviewed against the Astropy-specific guidelines listed below.

(astropy-collaboration)=
Copy link
Contributor

Choose a reason for hiding this comment

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

What is (astropy-collaboration)=? What should I be expected to see with this directive in the rendered page?

partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
partners/astropy.md Show resolved Hide resolved
partners/astropy.md Outdated Show resolved Hide resolved
@pllim
Copy link
Contributor

pllim commented Feb 13, 2024

p.s. Ah, my bad, I didn't realize rendered page is a different entry in the CI listing. I can see it here: https://output.circle-artifacts.com/output/job/1cb7525e-e3e6-4a4a-bec7-97aec45e29da/artifacts/0/html/partners/astropy.html

I think you need a logo that can adapt to dark mode?

Screenshot 2024-02-13 153325

We have one here: https://github.com/astropy/astropy/blob/main/docs/_static/astropy_banner_96_dark.png (if you don't want that one, check out others in https://github.com/astropy/astropy/tree/main/docs/_static )

Thanks!

@lwasser lwasser mentioned this pull request Feb 20, 2024
8 tasks
@lwasser
Copy link
Member Author

lwasser commented Feb 20, 2024

ok i've integrated all of your edits @pllim . i understand this feels repetitive. however, for now i'd like to have overlapping language here and in astropy only because we can then internally link in our review template to this page in the guidebook for consistency and also ensure our partnerships are consistent. if we find in the future that dedicating an entire page is over doing it / difficult to co-maintain we can always revisit. but i find these pages are also useful for new partners to look at to see how we're leveraging the review process and working together.

what do you think the next step is in terms of enough approval for us to merge this?

@lwasser
Copy link
Member Author

lwasser commented Feb 21, 2024

NOTE: i have to update the logo to include the dark mode adapted one! and then we can potentially merge this is astropy folks are on board with that.

We have one here: https://github.com/astropy/astropy/blob/main/docs/_static/astropy_banner_96_dark.png (if you don't want that one, check out others in https://github.com/astropy/astropy/tree/main/docs/_static )

@lwasser
Copy link
Member Author

lwasser commented Feb 28, 2024

https://github.com/astropy/astropy/blob/main/docs/_static/astropy_banner_96_dark.png

hey @pllim et al. so the image on that page is just a markdown image - its not managed with styles. as such i really don't know how to implement an astropy logo that works on both dark and light mode without creating something new. our book theme has both dark and light mode so the logo will need to look good on both. right now the one with the white background is the only one that renders properly. the others are either for dark or light mode but i'm not sure both.

:::{figure-md} astropy-logo
:class: myclass

<img src="/images/astropy_project_logo.svg" alt="The astropy logo. The logo is an orange egg shape with a white snake spiraling in it. The text of the image says The Astropy Project on three lines." class="mb-1" width="400px">

:::

below is what things look like in dark vs light mode

Screenshot 2024-02-27 at 5 11 07 PM Screenshot 2024-02-27 at 5 11 01 PM

i'm happy to change the image but i'd need someone to provide me with an image that works in both cases OR if you know how to manage dark and light mode within a markdown image call that still has alt text, etc that could work too? otherwise we'll have to stick to the one with the white background.

@lwasser
Copy link
Member Author

lwasser commented Feb 28, 2024

i just noticed i am not linking to your affiliated packages language on the Astropy website. so i just pushed a small commit ensuring that link as well. I also fixed case - capital A when discussing the project / community / lower case for the package.

@pllim
Copy link
Contributor

pllim commented Feb 28, 2024

Thanks for the cross-link!

It is okay if you can't get dark mode logo to work; not the end of the world. Thanks for trying!

Comment on lines 12 to 20
If you are submitting a package for review and wish to also apply for Astropy affiliation, your package will be:

* Reviewed against current pyOpenSci guidelines and checks.
* Also reviewed against the Astropy-specific guidelines listed below.

If you are submitting a package for review and wish to also apply for Astropy affiliation, your package will be:

- Reviewed against current pyOpenSci guidelines and checks.
- Also reviewed against the Astropy-specific guidelines listed below.
Copy link

Choose a reason for hiding this comment

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

This text is duplicated, just with different symbols ("-" vs "*").

Copy link
Member Author

Choose a reason for hiding this comment

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

oh gosh - good catch. i'll fix that.

Copy link
Member Author

Choose a reason for hiding this comment

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

that must have been a merge conflict fix gone wrong is my only guess 😆

@lwasser
Copy link
Member Author

lwasser commented Feb 28, 2024

Thanks for the cross-link!

It is okay if you can't get dark mode logo to work; not the end of the world. Thanks for trying!

thanks @pllim yes i'm just not sure how to deal that. i'm not a fan of these themes with light and dark mode as they are so much harder to maintain well. Things like this come up a lot! i wonder if i can force the theme to remove dark mode altogether


## Long-term maintenance & affiliate status over time

All packages accepted into the pyOpenSci ecosystem will follow the [pyOpenSci
Copy link
Member Author

Choose a reason for hiding this comment

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

@pllim @hamogu i made this change just now. I added this statement that all packages follow our pyOpenSci policies first (which we co-created together a few months ago here. And then added the Astropy specific standards for affiliation.

@pllim
Copy link
Contributor

pllim commented Feb 29, 2024

My approval stands. @dhomeier , would be nice if you can approve too. Thanks, all!

@lwasser
Copy link
Member Author

lwasser commented Mar 1, 2024

ok y'all. It's march 1 - merge day! merging this!!

@lwasser lwasser merged commit 75cc5e8 into pyOpenSci:main Mar 1, 2024
3 of 4 checks passed
@lwasser lwasser deleted the astropy-partnership branch March 1, 2024 19:19
@pllim
Copy link
Contributor

pllim commented Mar 1, 2024

👏 👏 👏

@dhomeier
Copy link

dhomeier commented Mar 4, 2024

Sorry, too much end-of-month business around here; looks great with all those review eyes gone over it.
Just the Ecosystem Integration is still a bit repetitive and might perhaps better be simply integrated as a sub-bullet into the Review Guidelines. Thanks everyone!

@pllim
Copy link
Contributor

pllim commented Mar 4, 2024

@dhomeier , this is already merged. Maybe submit your suggestion as a PR? Thanks!

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.

5 participants