Docstrings Review #297
Docstrings Review #297
Conversation
| - `axis`: (`::String`, `="axial"`, opts=[`"axial"`, `"coronal"`, `"sagittal"`]) orientation | ||
| of the phantom |
There was a problem hiding this comment.
I was simply trying to adhere to the 92-character line length limit.
| @@ -83,7 +83,7 @@ Sequence() = Sequence([Grad(0, 0)]) | |||
| """ | |||
| str = show(io::IO, s::Sequence) | |||
There was a problem hiding this comment.
This function does not return a string. The same for ALL show functions.
There was a problem hiding this comment.
Addresed in b1f8e54
| y = is_RF_on(x::Sequence) | ||
| y = is_RF_on(x::Sequence, t::Vector{Float64}) | ||
| y = is_RF_on(x::Sequence, t::Vector{Real}) | ||
|
|
There was a problem hiding this comment.
Rename x to seq everywhere in the docstrings (if x is a Sequence).
Rename x to rf everywhere in the docstrings (if x is an RF).
Rename x to gr everywhere in the docstrings (if x is a Grad).
Rename x to adc everywhere in the docstrings (if x is an ADC).
Rename x to delay everywhere in the docstrings (if x is a Delay).
There was a problem hiding this comment.
Addressed in 949850c
There was a problem hiding this comment.
General comment (major revision).
Please make sure that the documentation is not more complex that the function; if the function is: f(x)=x^2, it does not need a docstring:
y = f(x)
A function that calculates the square of a number.
# Arguments
- `x`: (`::Number`) input number
# Output
- `y`: (`::Number`) output number
# Example
...
This is true for the show functions and others, go one-by-one and ask the question "Is this docstring unnecessarily complex for the function?". I would argue that the only functions that need more in-depth documentation are the exported functions. If there is more docstring that code, we have a problem; do not bloat the code.
There was a problem hiding this comment.
I agree that should be docstring when it is not necessary.
I did the excercise you proposed, however I couldn't find more autodescriptive functions which wouldn't require a docstring, according to my criteria.
For example, this one:
Rx = rotx(θ::Real)
Rotates a three-dimensional vector or matrix with three rows counter-clockwise with respect
to the x-axis.
# Arguments
- `θ`: (`::Real`, `[rad]`) rotation angle
# Returns
- `Rx`: (`::Matrix{Real}`) rotation matrix
This function looks like is simple enough to don't require a docstring, however the units are important and the direction of the rotation too.
Maybe, I'm a little bit biased since I also try to keep uniformity in the docsstrings.
I eliminated the docstrings for Base.show methods in b1f8e54
|
Also this branch is not passing for Julia 1.9 |
Co-authored-by: Carlos Castillo Passi <cncastillo@uc.cl>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #297 +/- ##
==========================================
- Coverage 92.60% 92.60% -0.01%
==========================================
Files 33 33
Lines 2245 2244 -1
==========================================
- Hits 2079 2078 -1
Misses 166 166
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Now it is passing Julia 1.9 and 1.10 (but fails on Nightly) |
This pull request is meant to address issue #260.