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

Jump to bottom

Add lakes alias to Figure.coast() #781

Merged
merged 10 commits into from
Jan 5, 2021
Merged

Add lakes alias to Figure.coast() #781

merged 10 commits into from
Jan 5, 2021

Conversation

willschlitzer
Copy link
Contributor

@willschlitzer willschlitzer commented Jan 2, 2021 •
edited
Loading

Adding the lakes alias for the -C argument to Figure.coast().

@willschlitzer willschlitzer marked this pull request as ready for review January 3, 2021 10:20
@willschlitzer willschlitzer added the documentation Improvements or additions to documentation label Jan 3, 2021
@willschlitzer willschlitzer added this to the 0.3.0 milestone Jan 3, 2021
@seisman
Copy link
Member

seisman commented Jan 3, 2021

Also see some discussions in #474 (comment).

Ping @GenericMappingTools/python for comments on the argument name.

seisman
seisman reviewed Jan 3, 2021
View reviewed changes
pygmt/base_plotting.py Outdated Show resolved Hide resolved
pygmt/base_plotting.py Outdated Show resolved Hide resolved
@willschlitzer @seisman
Update pygmt/base_plotting.py
825484b 
Co-authored-by: Dongdong Tian <[email protected]>
@willschlitzer
Copy link
Contributor Author

@seisman Looking at that PR, I'm not sure what the issue with using a long-name alias for C is. It seems like the main concern is backwards compatibility, but wouldn't scripts still be able to use C instead of the alias?

@seisman
Copy link
Member

seisman commented Jan 4, 2021

It seems like the main concern is backwards compatibility, but wouldn't scripts still be able to use C instead of the alias?

Yes, C always works.

We need to be very careful about choosing these long names. Sometimes we may find that the long names are inappropriate and have to change them to other names, which may break backward compatibility. For example, @weiji14 and I were thinking about changing sizes to size in plot() (#666 (comment)).

@seisman
Copy link
Member

seisman commented Jan 4, 2021

Just checked the documentation of coast(), I think lakes is a good name, considering that we already use "rivers", "shorelines" and "borders".

@seisman
Copy link
Member

seisman commented Jan 4, 2021

/format

seisman
seisman approved these changes Jan 4, 2021
View reviewed changes
Copy link
Member

@seisman seisman left a comment

Choose a reason for hiding this comment

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

OK to merge after you fix this style issue:

pygmt/base_plotting.py:117:80: W505 doc line too long (83 > 79 characters)

@willschlitzer willschlitzer merged commit a40a075 into GenericMappingTools:master Jan 5, 2021
@willschlitzer willschlitzer deleted the coast-lakes-alias branch January 5, 2021 07:32
@seisman
Copy link
Member

seisman commented Jan 5, 2021

@willschlitzer When merging a PR, it would be better to simplify the commit description. For example, this is how this PR looks like after merging into the master branch.

image

A better one is:

image

@willschlitzer
Copy link
Contributor Author

@seisman Sounds good; my apologies for just leaving the defaults in.

@seisman seisman added enhancement Improving an existing feature and removed documentation Improvements or additions to documentation labels Jan 18, 2021
sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
@willschlitzer @seisman
Add lakes alias to Figure.coast() (GenericMappingTools#781)
9758d41 
* Add lakes alias to coast in base_plotting.py

* Changing color to fill

* Update coast docstring to r-string and change line lengths

* Add test for lake alias to test_coast.py

* Run make format

* Update pygmt/base_plotting.py

Co-authored-by: Dongdong Tian <[email protected]>

* Update pygmt/base_plotting.py

Co-authored-by: Dongdong Tian <[email protected]>

* Fixing doc string length

Co-authored-by: Dongdong Tian <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seisman seisman seisman approved these changes

Assignees
No one assigned
Labels
enhancement Improving an existing feature
Projects
None yet
Milestone
0.3.0
Development

Successfully merging this pull request may close these issues.
2 participants
@willschlitzer @seisman