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

T66 docs for tags and settings #355

Merged
merged 5 commits into from
Aug 15, 2024
Merged

T66 docs for tags and settings #355

merged 5 commits into from
Aug 15, 2024

Conversation

frisograce
Copy link
Collaborator

@frisograce frisograce commented Jul 16, 2024

Please check the following before creating the pull request (PR):

  • Did you run automatic tests?
  • Did you run manual tests?
  • Is the code provided in the PR still backwards compatible to previous SIMPA versions?

List any specific code review questions
Hi, just saw @TomTomRixRix 's recent comment about wanting more documentation for the tags and settings. Coincidentally I'd already written a little to help me understand it when I first arrived, so I've polished it a little and thought it may help.

Questions:

  1. Is this detailed enough?
  2. Is it right for it to have it's own documentation file?
  3. How do I make it so that the specific Tags documentation is linked?

Provide issue / feature request fixed by this PR

Fixes #66

@frisograce frisograce changed the title T66 docs for tags settings T66 docs for tags and settings Jul 16, 2024
@TomTomRixRix
Copy link
Collaborator

Hi, this is exactly what @seitela and I had in mind when writing the comment in #66. Like this the user has a resource to understand the concept behind these tags and settings. Regarding your questions:

Is this detailed enough?

I would say yes, maybe even a bit too detailed for lazy documentation readers who would only need an abstract to understand the concept. But having some examples is helpful I guess and the reader can still decide if they want to read the entire documentation.

Is it right for it to have it's own documentation file?

If would prefer having a short summary in the README.md like for the Path Management which includes a link to the detailed documentation file. But I think that this is just my personal taste and we might have to discuss this in a meeting to get multiple opinions.

How do I make it so that the specific Tags documentation is linked?

I guess you would like to link this page? I'm not sure but maybe we can set an internal link to this documentation page, however, I don't have any experience with this.

TomTomRixRix
TomTomRixRix previously approved these changes Jul 18, 2024
Copy link
Collaborator

@TomTomRixRix TomTomRixRix left a comment

Choose a reason for hiding this comment

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

I didn't test building the documentation but content-wise it looks good 👍

@TomTomRixRix
Copy link
Collaborator

I'm currently working on #29 and refactor all the default settings into the respective adapters so that in the future the user only has to specify settings which deviate from the sensible defaults. We should add this to the documentation once it's implemented.

@TomTomRixRix
Copy link
Collaborator

An interesting information about the Settings which I think we should document is whether they are read only or can be manipulated by the simulation modules. For example, currently the simulate method write the currently simulated wavelength into the global settings which is important to know.

@frisograce frisograce added the documentation Improvements or additions to documentation label Jul 30, 2024
jgroehl
jgroehl previously approved these changes Aug 13, 2024
@kdreher kdreher closed this Aug 15, 2024
@kdreher kdreher reopened this Aug 15, 2024
@kdreher kdreher merged commit 85463ef into develop Aug 15, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants