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.

Sign up for GitHub

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

Documenting dev policy: github issue linking #1865

Merged
merged 2 commits into from
Sep 12, 2024
Merged

Documenting dev policy: github issue linking #1865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91563b3
Documenting dev policy: github issue linking
john-science Sep 10, 2024
7b371fd
Removing link to 4-year-old, abandonded ticket
john-science Sep 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions armi/bookkeeping/visualization/xdmf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,7 @@
wedges. To do that would require splitting the parameter data, which would defeat
the main benefit of using XMDF in the first place (to be able to plot out of the
original Database file). Cartesian and R-X-Theta geometries in VisIt seem to work
fine. Support for polyhedra is being tracked in `#1287
<https://github.com/visit-dav/visit/issues/1287>`_.
fine.
"""

import io
Expand Down
18 changes: 12 additions & 6 deletions doc/developer/standards_and_practices.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ functions, and methods and their signatures (the signature includes the paramete

* Variables that you designate as unused should be prefaced with an underscore (``_``).
* Do not use Python `reserved keywords <https://realpython.com/lessons/reserved-keywords/>`_ as variable names.
* Try to use names that are pronounceable. (Well-established variable names from equations are exceptable.)
* Try to use names that are pronounceable. (Well-established variable names from equations are acceptable.)
* Keep names concise and expressive. (An exception is test method names, which may be longer and more
descriptive.)
* Avoid abbreviations and acronyms, unless they are well understood by subject-matter experts (e.g. DB for database,
Expand Down Expand Up @@ -195,7 +195,7 @@ Avoid repeating code
====================
In other words, don't repeat yourself. (`D. R. Y. <https://en.wikipedia.org/wiki/Don't_repeat_yourself>`_).
Repetitious code is harder to read, and harderd for others to update. If you ever find yourself copying and pasting
code, consider pulling the repeated code out into it's own function, or using a loop.
code, consider pulling the repeated code out into its own function, or using a loop.

Public methods should have docstrings
=====================================
Expand Down Expand Up @@ -298,17 +298,23 @@ Input files
ARMI developers **shall** use one of the following well-defined, Python-supported, input file formats.

.json
JSON files are used for a variety of data-object representations. There are some limitations of JSON, in that it
does not easily support comments. JSON is also very strict.
JSON files are used for a variety of data-object representations. There are some limitations of
JSON, in that it does not easily support comments. JSON is also very strict.

.yaml
YAML files are like JSON files but can have comments in them.

General do's and don'ts
=======================

do not use ``print``
Do not use ``print``
ARMI code should not use the ``print`` function; use one of the methods within ``armi.runLog``.

Do not add new ``TODO`` statements in your commits and PRs.
If your new ``TODO`` statement is important, it should be a GitHub Issue. Yes, we have existing ``TODO`` statements in the code, those are relic and need to be removed. Also, never mark the code with ``FIXME`` or ``XXX```; open a ticket.
If your new ``TODO`` statement is important, it should be a GitHub Issue. Yes, we have existing
``TODO`` statements in the code, those are relic and need to be removed. Similarly, never mark
the code with ``FIXME`` or ``XXX```; open a ticket.

Do not link GitHub tickets or PRs in code.
The idea in ARMI is that either something is worth documenting well in a docstring, or the docs,
or it is not. And just linking a ticket or PR in a docstring is not helpful.
Comment on lines +318 to +320
Copy link
Contributor

Choose a reason for hiding this comment

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

I've found external links to be helpful as supplemental resources. An empty link is not helpful, yes. But it can provide people references to go digging later if they want. Or for more context behind the change.

In the case of the material theoretical density changes that kicked off this work, there is a lot of good discussion in #1440. And that context and discussion motivates some strange fixes that a future developer may have questions about. Putting some explanation in comments or docstrings could be sufficient, but is that "documenting well?"

Alternatively, I can do a git blame and find the commit that introduced a change with interesting commentary. And then find the PR that contains that commit, find the issue that PR closes, and then I have some more context and background on a change.

There's some motivation to make everything self sufficient (e.g., "without recourse to the originator") that I get.

Is there a balance that can be struck? Linked issues can be stale or wrong and that's less helpful. But they can be quick ways to provide useful background. Maybe

Do not add bare links to external tickets or change requests in code.
    A link to a GitHub issue on its own is not helpful. Provide enough information via comments or
    docstrings that a passing developer does not need to search for more. Links should be used as
    supplemental resources only, with the bulk of the context being added in the source code.

?

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, my notion goes like this:

  1. We have managed for a decade without doing this yet.
  2. I have seen projects that do this, and inevitably, their docstrings and internal docs suffer hard.
  3. I personally, me, don't want to have to go do homework when I'm reading the code. It takes what should be reading a one-sentence docstring to reading a 4-page story.
  4. I don't think "background" and "history" help people understand code. If you can't say "This works like this, because of this" then just giving an infinite amount of context and history and background is just telling people you can't document the feature clearly, and are giving up.

BUT I will admit there is a lot of personal bias above, so let me think about this some more.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, it occurs to me that GitHub is the third git platform/tool this project has been on.

Pointing to a GitHub ticket isn't particularly useful if we've moved on to GitLab or whatever the new hotness is in 6 years.

Copy link
Contributor

Choose a reason for hiding this comment

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

All great points. I don't have any good rebuttals. And we should definitely encourage any documentation to stand on its own

Loading