wrapper for gmt info #106
wrapper for gmt info #106
Conversation
gmt/__init__.py
Outdated
| # Import modules to make the high-level GMT Python API | ||
| from .session_management import begin as _begin, end as _end | ||
| from .figure import Figure | ||
| from .modules import * |
There was a problem hiding this comment.
F403 'from .modules import *' used; unable to detect undefined names
There was a problem hiding this comment.
Please don't import *. It's better to explicitly import the functions that should be public.
| from .utils import build_arg_string | ||
| from .decorators import fmt_docstring, use_alias, kwargs_to_strings | ||
|
|
||
| @fmt_docstring |
There was a problem hiding this comment.
E302 expected 2 blank lines, found 1
|
@seisman mostly looks good to me. I have some minor comments that I'll leave inline. |
gmt/modules.py
Outdated
| Parameters | ||
| ---------- | ||
| data: str | ||
| A data file name. |
There was a problem hiding this comment.
Let's keep it only for data files for now. We can think about adding pandas.Dataframe later but I don't know if it will be helpful.
In this case, fname is a better name for the argument. I'll probably refactor plot to use fname for files as well.
gmt/modules.py
Outdated
| arg_str = ' '.join([data, build_arg_string(kwargs), | ||
| "->" + tmpfile.name]) | ||
| with LibGMT() as lib: | ||
| lib.call_module('gmtinfo', arg_str) |
There was a problem hiding this comment.
I don't think you need the "gmt" in the name.
gmt/modules.py
Outdated
| """ | ||
| assert isinstance(fname, str), 'Only accepts file names.' | ||
|
|
||
| with GMTTempfile() as tmpfile: |
| """ | ||
| assert isinstance(fname, str), 'Only accepts file names.' | ||
|
|
||
| with GMTTempFile() as tmpfile: |
|
Maybe I need to add more tests? |
|
|
||
| TEST_DATA_DIR = os.path.join(os.path.dirname(__file__), 'data') | ||
|
|
||
| def test_gmtinfo(): |
There was a problem hiding this comment.
E302 expected 2 blank lines, found 1
@seisman yes please! I try to have as many configurations as possible tested. The one you have now looks good for the basic functionality. Would you mind adding some more information to the docstring? You can copy some of the basic descriptions from the GMT docs. Not all options need to be included, just the most common ones. We can think of some aliases for them as well. Do you think we should print the output instead of returning it? There could be a flag to trigger a return instead of printing. Either way, if the returned string is just a list of numbers, it would be better to return an array/list of the number instead. |
3 participants
Fix #43. Still work in progress. Any suggestions?