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

Include SasData documentation in SasView #2672

Merged
merged 8 commits into from
Dec 5, 2023

Conversation

krzywon
Copy link
Contributor

@krzywon krzywon commented Nov 13, 2023

Description

This PR integrates the documentation generated by the sasdata package into SasView using build_sphinx.py and CI.

This is reliant on the work in SasView/sasdata#53 and uses the associated sasdata branch in ci.yml.

How Has This Been Tested?

CI output

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)
  • User documentation is available and complete (if required)
  • Developers documentation is available and complete (if required)
  • The introduced changes comply with SasView license (BSD 3-Clause)

Copy link
Contributor

@llimeht llimeht left a comment

Choose a reason for hiding this comment

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

Looks good.

  • I assume 29: Building sasdata documentation sasdata#53 will get get merged first and then ci.yml will get updated prior to merging this
  • Is it worth folding the sphinx-apidoc and sphinx-build calls that are duplicated here and in sasdata/.github/workflows/test.yml into the doc/Makefile? (in the same way as bumps and sasmodels have done so that it's a uniform interface to the three)
  • retrieve_sasdata_docs is currently necessary but this sort of layout is not nice to new contributors with the complexity it brings to getting started with sasview, also not nice for downstream people who will have to come up with ways of circumventing it.

All this work reminds me of #2421 again btw.

@krzywon
Copy link
Contributor Author

krzywon commented Nov 14, 2023

That's the expectation.

  • Is it worth folding the sphinx-apidoc and sphinx-build calls that are duplicated here and in sasdata/.github/workflows/test.yml into the doc/Makefile? (in the same way as bumps and sasmodels have done so that it's a uniform interface to the three)

Good catch. I missed that. I'll make those changes.

All this work reminds me of #2421 again btw.

Agreed. At the last code camp, we started to look into combining everything into a single repository (sasmodels and sasdata included) and using CI to create separate releases for each 'external' repo. This would eliminate a lot of this hassle, including your first point, but continue to allow a cleaner separation of the different code sections. That is still a WIP.

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Nov 19, 2023
@wpotrzebowski
Copy link
Contributor

Tested on Mac. Works fine. In my opinion we should merge it

@wpotrzebowski
Copy link
Contributor

@krzywon any objections to merging it?

@wpotrzebowski wpotrzebowski added Discuss At The Call Issues to be discussed at the fortnightly call and removed Discuss At The Call Issues to be discussed at the fortnightly call labels Dec 3, 2023
@krzywon
Copy link
Contributor Author

krzywon commented Dec 4, 2023

@wpotrzebowski, SasView/sasdata#53 must be first merged and then I need to point the CI here to sasdata master.

@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Dec 5, 2023
@wpotrzebowski
Copy link
Contributor

Tested on Mac. As discussed at the call I am merging it.

@wpotrzebowski wpotrzebowski merged commit 647a580 into release_6.0.0 Dec 5, 2023
36 checks passed
@wpotrzebowski wpotrzebowski deleted the sasdata-29-documentation branch December 5, 2023 21:21
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