Skip to content

Conversation

zmx27
Copy link
Contributor

@zmx27 zmx27 commented Aug 4, 2025

No description provided.

@zmx27
Copy link
Contributor Author

zmx27 commented Aug 4, 2025

@sbillinge this is ready for review. This also addresses the error that vince ran into at the following comment:
#66 (comment)

Done in collaboration with @ycexiao

Copy link
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

this looks good. couple of small comments

@@ -64,7 +72,7 @@
source_suffix = [".rst", ".md"]

# The encoding of source files.
# source_encoding = 'utf-8-sig'
# source_encoding = eutf-8-sig'
Copy link
Contributor

Choose a reason for hiding this comment

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

is this intended?

@@ -134,7 +142,7 @@
# The theme to use for HTML and HTML Help pages. See the documentation for
# a list of builtin themes.
#
html_theme = "sphinx_rtd_theme"
Copy link
Contributor

Choose a reason for hiding this comment

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

lets comment out sphinx_rtd_theme in case we want to go back some time

@zmx27
Copy link
Contributor Author

zmx27 commented Aug 5, 2025

@sbillinge ready for review

@sbillinge
Copy link
Contributor

great work getting to the bottome of this! Thanks so much.

@sbillinge sbillinge merged commit 20799ab into diffpy:migration Aug 5, 2025
2 checks passed
@sbillinge
Copy link
Contributor

readthedocs successfully built the docs now, but they are not rendering correctly for some reason. It almost looks like they are rendering an older version. @zmx27, @ycexiao please could you look into it and see what you can come up with?

@vincefn
Copy link
Collaborator

vincefn commented Aug 5, 2025

Docs are well rendered on https://pyobjcryst.readthedocs.io/en/migration/. Nice. Interesting to see how you got rid of nbsphinx_lnk.

What I can see:

  • the QPA notebook is not listed
  • Can the "Getting started" part be removed or updated - it seems like a template for docs - that's not where we want people to start with pyobjcryst ?
  • the release notes are not up-to-date
  • main page says "no version found" - not sure what the mechanism is to find it.

Cheers,

@ycexiao
Copy link
Contributor

ycexiao commented Aug 5, 2025

readthedocs successfully built the docs now, but they are not rendering correctly for some reason.

I think the one from migration branch is our latest version https://pyobjcryst.readthedocs.io/en/migration/. It agrees with my local built documentation.

main page says "no version found" - not sure what the mechanism is to find it.

Yes, and the API is also not successfully built. It is because pyobjcryst is not installed when readthedocs build the documentation. I asked chatgpt and it suggests to add something like

# readthedocs.yml
version: 2

build:
  os: ubuntu-22.04
  tools:
    python: "3.11"

python:
  install:
    - method: pip
      path: .

in the readthedocs.yml

the release notes are not up-to-date

The content in the release notes is from CHANGELOG.rst in the migration branch. I think it is the latest version, but we can also modify the file to update it.

@ycexiao ycexiao mentioned this pull request Aug 5, 2025
@ycexiao
Copy link
Contributor

ycexiao commented Aug 5, 2025

Since it is a merged PR, I have summarized our current issues in #66 (comment)

@sbillinge
Copy link
Contributor

@vincefn I think we have addressed most things. Please can you take a look again

* the QPA notebook is not listed

This is fixed in the latest version

* Can the "Getting started" part be removed or updated - it seems like a template for docs - that's not where we want people to start with pyobjcryst ?

This has been removed. We should have done that before. It is a template to help people with new projects.

* the release notes are not up-to-date

Could you elaborate what you mean by this? Is it that changes that have been made on this migration branch are not showing up? These will appear when we make a non rc release. In our workflow these are stored as rst files in news. When we make a full (non-rc) release, these are automatically compiled into the changelog. So it should all appear automatically at that time.

* main page says "no version found" - not sure what the mechanism is to find it.

this is correct. Since this is not a proper released version we don't want it to identify with a release. We do this for pre-releases too. When the full release goes out (2025.1.0) it will show that in that location.

Thanks so much for your eyes on this. I think we are nearly there.

If you are happy with this now, I will merge migration into main and push out a full release and we can keep our fingers crossed it all works!

I have a couple of other fixes....@zmx27 please could you make a PR for this.

  1. I want to change the text that this "maintained as part of the Diffpy-CMI project at Brookhaven National Laboratory" to saying it is "maintained as part of the diffpy project". We don't need to mention any lab or university. This will need to be changed in the docs and in the readme if it is there.
  2. Let's double-check that the installatoin instructions in the README are what we want. We would like conda-forge installation instructions. Also we want to have instructions to install from sources using scons, but further down as this is just for advanced users. @vincefn do we want instructions how to install from PyPI also? This is possible in principle but more effort because the dependencies have to installed explicitly. We may want to not bother with those instructions?

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

Successfully merging this pull request may close these issues.

4 participants