-
Notifications
You must be signed in to change notification settings - Fork 31
Wipe cycstub repo #419
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
Wipe cycstub repo #419
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me. I recommend some of the capitalization is consistent with 1856 in Cyclus.
cycstub --type facility :stublibrary:StubFacility | ||
cycstub --type institution :stublibrary:StubInstitution | ||
cycstub --type region :stublibrary:StubRegion |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want these to have CamelCase in the library name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it this way before I committed to fixing the CamelCase problem. In the fixed version, all combinations will work successfully, so these don't need to change.
source/arche/hello_world_cpp.rst
Outdated
@@ -4,48 +4,32 @@ Hello, Cyclus! [C++] | |||
==================== | |||
This pages walks you through a very simple hello world example using | |||
|cyclus| agents. First make sure that you have the dependencies installed, | |||
namely |Cyclus|, CMake, and a recent version of Python (2.7 or 3.3+). | |||
namely |Cyclus|, CMake, and a recent version of Python (3.3+). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what our minimum version is these days????
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My guess would be python 3.9, but I can look around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going off of the dependencies for installing cyclus via conda, it looks like 3.8 is the lowest version allowed. I can build cyclus with 3.8 locally, but the unit tests won't pass unless you have python 3.11+, as of commit 4d1eeef5
I reverted this to draft because a little more work needs to be done to make it all consistent and useful. |
I think this is probably where this needs to be to be consistent now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only two comments I have are the same PascalCase vs camelCase comment I made in the other PR #1856, and that (and I'm not sure if this really needs to be fixed), but a naive version of me would probably try to type the command tutorial $ cycstub --type inst tutorial:TutorialLibrary:TutorialInstitution
instead of the command cycstub --type inst tutorial:TutorialLibrary:TutorialInstitution
in the tutorial folder. Perhaps if you copy the command from the markdown file it is smart enough to not give you that part, but just a thought! Otherwise this looks good to me.
I think we've often wondered whether it makes sense to include the prompt in the samples. I'm torn on this. The only real benefit is that you can see the directory change and the user gets some clear feedback on which directory they should be in to run the command. The downside is that it looks like part of the command... we could try a different prompt separator... maybe:
|
I'm not sure the separator is really the problem, since ultimately this "issue" is with people who aren't that familiar with using the terminal. I wonder how other people handle this when writing documentation. I guess my personal preference is to change: """
""" Into something like: """ From the "tutorial" directory, run:
""" But perhaps that's too verbose/much overhead/easy to miss? Ultimately I don't think it matters much (except that actually this makes the commands really easy to copy form the markdown file and paste in your terminal since you don't have to remove the |
I just updated all the |
A few typos and copyedit improvements Co-authored-by: Amanda Bachmann <[email protected]>
5f0bae3
to
6ad43cf
Compare
All the bash code blocks now have sytnax highlighting that requires the prompt to be indicated with only a single |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gonuke for making that change. This looks good to me now. I'll leave it unmerged until Wednesday morning to give others a chance to look at it on Monday if they want (I know @abachma2 mentioned wanting some time after TWOFCS to check things). If others get around to it before then, and approve, feel free to merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @gonuke. I am approving this PR. I have one comment to follow up on the minimum python version. If we don't have any more conversation about that then I will merge.
source/arche/hello_world_cpp.rst
Outdated
@@ -4,48 +4,32 @@ Hello, Cyclus! [C++] | |||
==================== | |||
This pages walks you through a very simple hello world example using | |||
|cyclus| agents. First make sure that you have the dependencies installed, | |||
namely |Cyclus|, CMake, and a recent version of Python (2.7 or 3.3+). | |||
namely |Cyclus|, CMake, and a recent version of Python (3.3+). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going off of the dependencies for installing cyclus via conda, it looks like 3.8 is the lowest version allowed. I can build cyclus with 3.8 locally, but the unit tests won't pass unless you have python 3.11+, as of commit 4d1eeef5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Merging now.
This removes references to the cycstub repo and instead replaces it with reference to the cycstub utility. This leaves one file untouched because it is slated to be removed in #418 .
cmake.rst
still needs a closer review, but this could be left to a different PR??Fixes #197 and #366