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

Refactor CommonGrids documentation #2086

Closed
wants to merge 1 commit into from
Closed

Conversation

charleskawczynski
Copy link
Member

This PR refactors the documentation in our CommonGrids module so that they can be reused (e.g., in our soon-coming common spaces).

@charleskawczynski charleskawczynski added the documentation Improvements or additions to documentation label Nov 16, 2024
@Sbozzolo
Copy link
Member

I understand that there's a lot of duplicated verbiage, but I personally think that this adds unnecessary complexity.

This change makes the docstrings partially unreadable from the source code, and introduces dynamism in the docstring (the documentation depends on the program), which means that one has to follow the code to understand what the docstring would contain. As a result, it also contains more points of failure and, I think, that makes updating the docstrings harder, not easier (especially to those who are not familiar with this docstring system yet).

As a middleground, I can suggest that we collect all the arguments in a single docstring for an empty variable const COMMON_ARGUMENTS_DOC, then, we add a For the definion of the various arguments, see [COMMON_ARGUMENTS_DOC](@ref) note to all the docstrings for grids/spaces. Docstrings are meant to be read by humans, so I think that this would still provide enough documentation for people to become familiar with the arguments

@charleskawczynski
Copy link
Member Author

I understand that there's a lot of duplicated verbiage, but I personally think that this adds unnecessary complexity.

I asked about how to reduce documentation like this on Julia slack, and this is what they suggested. I agree that it's not ideal, but it seems better to me than maintaining two (pretty long and duplicate) lists of documentation arguments and descriptions.

This change makes the docstrings partially unreadable from the source code, and introduces dynamism in the docstring (the documentation depends on the program), which means that one has to follow the code to understand what the docstring would contain. As a result, it also contains more points of failure and, I think, that makes updating the docstrings harder, not easier (especially to those who are not familiar with this docstring system yet).

I think it's easier to maintain, though.

As a middleground, I can suggest that we collect all the arguments in a single docstring for an empty variable const COMMON_ARGUMENTS_DOC, then, we add a For the definion of the various arguments, see [COMMON_ARGUMENTS_DOC](@ref) note to all the docstrings for grids/spaces. Docstrings are meant to be read by humans, so I think that this would still provide enough documentation for people to become familiar with the arguments

My issue with doing this is that users will still have a second thing to lookup how something works.

My preference would be to either

  • Follow through with this set of changes, and then we can reuse these docs in CommonSpaces
  • Leave these docs as they are, and leave CommonSpaces as it is

@Sbozzolo
Copy link
Member

I think it's easier to maintain, though.

I disagree. I think it's easier to maintain duplicated inert strings than code that has some behavior that needs to be understood by the reader.

If you don't want to have the shared argument docstring, my preference would be to keep it as it is.

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Nov 18, 2024

If you don't want to have the shared argument docstring, my preference would be to keep it as it is.

Alright, should we merge #2082, then? I can close this PR.

@charleskawczynski
Copy link
Member Author

I spoke with @Sbozzolo, and we're just going to keep things simple and duplicate the doc strings.

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.

2 participants