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

Replace HTTP URLs with HTTPS, avoid from pytest import raises/mark #4021

Merged
merged 18 commits into from
Aug 30, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Aug 28, 2024

Summary

  • Replace HTTP URLs with HTTPS
  • Replace some expired URLs
  • Avoid from pytest import raises/mark (a bit unsure about MonkeyPatch so just use pytest.MonkeyPatch if pytest is already imported)
  • Regenerate documents

@@ -56,7 +56,7 @@ def lattice_from_abivars(cls=None, *args, **kwargs):
raise ValueError(f"The sum of angdeg must be lower than 360, {ang_deg=}")

# This code follows the implementation in ingeo.F90
# See also http://www.abinit.org/doc/helpfiles/for-v7.8/input_variables/varbas.html#angdeg
Copy link
Contributor Author

@DanielYang59 DanielYang59 Aug 28, 2024

Choose a reason for hiding this comment

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

I have to replace this with the latest doc (doesn't seem to be versioned) instead of the original v7.8, because the original URL seems inaccessible.

@DanielYang59 DanielYang59 changed the title Replace http URLs with https Replace HTTP URLs with HTTPS Aug 28, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review August 28, 2024 13:27
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Aug 28, 2024

@janosh Please review this. Looks like we might need to regenerate docs, thanks!

I reverted the rebuild doc commit because it makes it impossible to review. Would let you do this, appreciate that!

@janosh
Copy link
Member

janosh commented Aug 28, 2024

Currently on my phone. Could you apply the docs commit again?

@@ -369,7 +369,7 @@ def to_snl(self, authors: list[str], **kwargs) -> StructureNL:
history += [
{
"name": snl_metadata.pop("name", "pymatgen"),
"url": snl_metadata.pop("url", "http://pypi.python.org/pypi/pymatgen"),
"url": snl_metadata.pop("url", "https://pypi.python.org/pypi/pymatgen"),
Copy link
Contributor Author

@DanielYang59 DanielYang59 Aug 29, 2024

Choose a reason for hiding this comment

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

This might potentially be a breaking change too (already reverted), the metadata (default value of pop for url would have been changed):

for hist in self.history:
snl_metadata = hist.pop("_snl", {})
history += [
{
"name": snl_metadata.pop("name", "pymatgen"),
"url": snl_metadata.pop("url", "http://pypi.python.org/pypi/pymatgen"),
"description": hist,
}
]

@DanielYang59 DanielYang59 changed the title Replace HTTP URLs with HTTPS Replace HTTP URLs with HTTPS, avoid from pytest import raises/mark Aug 30, 2024
@DanielYang59 DanielYang59 requested a review from janosh August 30, 2024 03:49
@janosh janosh added docs Documentation, examples, user guides security Security issues or fixes labels Aug 30, 2024
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

ideally, we should have a link checker CI similar to pymativz to make sure all the new (and old links) are actually still alive. but i'm happy to leave that to a future PR. maybe something to open an issue for so it's not forgotton

@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Aug 30, 2024

ideally, we should have a link checker CI similar to pymativz to make sure all the new (and old links) are actually still alive. but i'm happy to leave that to a future PR. maybe something to open an issue for so it's not forgotton

Wonderful. Added to my TODO list (which is not very long at this moment :) ). I would need some investigation on that because markdown-link-check seemingly just check markdown files.

Can you regenerate documents after reviewing? Thanks!

@janosh
Copy link
Member

janosh commented Aug 30, 2024

@DanielYang59 do you want to reapply the docs update commit before i merge?

@DanielYang59
Copy link
Contributor Author

@janosh certainly, got two warnings at the start, not sure if we should worry about that or not (it seems favicon.ico and html_static_path = ["assets"] don't exist?):

>>> invoke make-doc
rm: pymatgen.*.rst: No such file or directory
Running Sphinx v7.4.7
loading translations [en]... done
making output directory... done
WARNING: html_static_path entry 'assets' does not exist
WARNING: favicon file 'favicon.ico' does not exist
Converting `source_suffix = '.rst'` to `source_suffix = {'.rst': 'restructuredtext'}`.
building [mo]: targets for 0 po files that are out of date
writing output... 
building [html]: targets for 49 source files that are out of date
updating environment: [new config] 49 added, 0 changed, 0 removed

@janosh
Copy link
Member

janosh commented Aug 30, 2024

@janosh certainly, got two warnings at the start, not sure if we should worry about that or not (it seems favicon.ico and html_static_path = ["assets"] don't exist?):

safe to ignore for now. the docs need some more work at some point but that would be a bigger project

@janosh janosh merged commit 934013b into materialsproject:master Aug 30, 2024
42 checks passed
@DanielYang59 DanielYang59 deleted the http-to-https branch August 30, 2024 06:03
@DanielYang59
Copy link
Contributor Author

Great to know! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation, examples, user guides security Security issues or fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants