-
Notifications
You must be signed in to change notification settings - Fork 224
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
Improve docstring of x2sys_init and x2sys_cross #930
Conversation
I see more tiny issues in the x2sys_cross documentation. Do you want to also fix those tiny issues in this PR? |
Sure, I can do that. |
Co-authored-by: Dongdong Tian <[email protected]>
I just converted this PR to draft to save CI resources. |
@seisman, is there a reason for this module to be in pygmt/, in contrast to the other modules which are in pygmt/src? |
It should be in See #832 (comment) and #807 (comment). |
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.
It should be in
src/
, but we haven't decided if we wantx2sys_init
andx2sys_cross
in one filesrc/x2sys.py
, or two filessrc/x2sys_init.py
andsrc/x2sys_cross.py
.See #832 (comment) and #807 (comment).
Just on this, maybe better to have them in two separate files, since there are other x2sys_*
modules too which may need to be wrapped in the future. Do we want to do the x2sys move before, during, or after this PR?
I agree about separate files and would prefer making that change during or after this PR to avoid conflicts and extra effort. |
Before or after? I can do one before but you'll need to resolve some merge conflicts, or I can open one after this one is merged. I presume this PR is basically done already? |
I realize now my language was confusing. By 'during' I really meant 'as part of', but in remembering the 'keep PR's small' policy it seems after is best. As far as I can tell this PR is done. |
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.
Ok, I had a look through the rendered x2sys_init
and x2sys_cross
API docs and they seem fine. Though best if @seisman can approve too since he has a better eye on the bold/italics/code-blocks matter.
Co-authored-by: Dongdong Tian <[email protected]>
Co-authored-by: Dongdong Tian <[email protected]>
/format |
1 similar comment
/format |
@meghanrjones |
All lines locally appear to be 79 characters or less and my branch is up to date. Should I request a re-run on the style checks? |
Lines 297 and 298 are too long.
|
) Co-authored-by: Dongdong Tian <[email protected]> Co-authored-by: Wei Ji <[email protected]>
Description of proposed changes
This fixes the bullet list in the
speed
parameter description in the x2sys_cross docs in order to suppress this sphinx build warning:Fixes #
Reminders
make format
andmake check
to make sure the code follows the style guide.doc/api/index.rst
.Slash Commands
You can write slash commands (
/command
) in the first line of a comment to performspecific operations. Supported slash commands are:
/format
: automatically format and lint the code/test-gmt-dev
: run full tests on the latest GMT development version