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

Small documentation fixes #1715

Merged
merged 19 commits into from Apr 15, 2024
Merged

Small documentation fixes #1715

merged 19 commits into from Apr 15, 2024

Conversation

abachma2
Copy link
Member

@abachma2 abachma2 commented Apr 1, 2024

This PR includes some small documentation fixes such as:

  • a note in the CONTRIBUTING that tells developers to update the CHANGELOG, addressing what was started in Mention necessity of creating news items for PRs. #1561
  • fix the formatting for cross referencing in INSTALL.rst hat renders correctly on the website and when viewed on GitHub
  • change conda package for dependencies to install only the dependencies for the cyclus package, allowing us to not have to maintain a core package and a dependencies package.
  • Change the branch name for guiding new developers in the README

@abachma2 abachma2 self-assigned this Apr 1, 2024
@abachma2 abachma2 added this to the v1.6 milestone Apr 1, 2024
Copy link

github-actions bot commented Apr 1, 2024

Downstream Build Status Report - 92d295b - 2024-04-15 09:09:15 -0500

Build FROM cyclus_20.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_20.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_apt/cyclus
  • Cycamore: Success
  • Cymetric: Success
Build FROM cyclus_22.04_conda/cyclus
  • Cycamore: Success
  • Cymetric: Success

@coveralls
Copy link
Collaborator

coveralls commented Apr 1, 2024

Pull Request Test Coverage Report for Build 8690624491

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 32.601%

Totals Coverage Status
Change from base Build 8512199629: 0.001%
Covered Lines: 53081
Relevant Lines: 129079

💛 - Coveralls

INSTALL.rst Outdated Show resolved Hide resolved
@abachma2
Copy link
Member Author

abachma2 commented Apr 2, 2024

Ok. So this isn't working for me either, even though I could have sworn it worked yesterday. The main issue we are running into here is that I am trying to find a format for internal hyperlinks that works when rendering this on the website and when you view it on GitHub. I don't think this is possible, because we would need one to point to a .rst file and the other to point to a .html file.

So a few ideas here:

  • we can keep the install files as separate things in each repo instead of getting the ones from the Cyclus and Cycamore repos so that we can make sure everything renders as needed
  • not worry about rendering on github (which is what we do in the website repo), and just link to the website.
  • We can have the website link to the file on Github, via the full web address, where everything renders correctly.

I am also open to other ideas.

@gonuke
Copy link
Member

gonuke commented Apr 3, 2024

I wonder if we can use Markdown files and this extension when we pull it into the website? No idea if it will generate URLs correctly. 🤷‍♂️

@bennibbelink
Copy link
Contributor

There must ways to get what we want if we add a pre-processing step. That extension might work, I'll see what else I can find.

@bennibbelink
Copy link
Contributor

After some research I think I'm in support of the third bullet by @abachma2:

We can have the website link to the file on Github, via the full web address, where everything renders correctly.

I haven't found a good way to make the link formatting work well in both Github and Sphinx contexts. We have a good way to link to an external reference (Github in this case), so I my suggestion is that we change the references at the bottom of INSTALL.rst to point to https://github.com/cyclus/cyclus/blob/main/DEPENDENCIES.rst instead of DEPENDENCIES.rst. It's not ideal but its only for these two instances, every other reference on the website to DEPENDENCIES should render fine.

@abachma2
Copy link
Member Author

abachma2 commented Apr 8, 2024

I pushed that change. Is there anything else I needed to change for this to get merged?

@abachma2
Copy link
Member Author

abachma2 commented Apr 8, 2024

And now that I've pushed this, we could also link to page on the website: https://fuelcycle.org/user/DEPENDENCIES.html. If y'all have a preference of one over the other.

@bennibbelink
Copy link
Contributor

Good point! I think referencing the page on https://fuelcycle.org would probably be better so we keep users in one place (although less robust).

@abachma2
Copy link
Member Author

Updated the link to the website instead of the github file. Is this ready to be merged now?

Copy link
Contributor

@bennibbelink bennibbelink 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 to me, thanks @abachma2!

@abachma2
Copy link
Member Author

If this is approved @bennibbelink, can we merge it? Or do you think we need another reviewer on it?

@bennibbelink
Copy link
Contributor

I saw the review from @gonuke is pending. Do you have a preference about linking to fuelcycle.org vs GitHub?

Copy link
Member

@gonuke gonuke left a comment

Choose a reason for hiding this comment

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

Just one typo to fix - then I'm happy with it.

CHANGELOG.rst Outdated Show resolved Hide resolved
@abachma2
Copy link
Member Author

Thanks for catching that typo @gonuke. Is this ready for merging now?

@abachma2 abachma2 requested a review from gonuke April 15, 2024 15:42
@nsryan2
Copy link

nsryan2 commented Apr 15, 2024

This PR closes #1713

@gonuke gonuke merged commit f7a5b52 into cyclus:main Apr 15, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants