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

Refactor text to use virtualfile_from_data #1121

Merged
merged 14 commits into from
May 22, 2022
Merged

Refactor text to use virtualfile_from_data #1121

merged 14 commits into from
May 22, 2022

Conversation

seisman
Copy link
Member

@seisman seisman commented Mar 25, 2021 •
edited
Loading

Description of proposed changes

  • Refactor text to use virtualfile_from_data
  • Raise exceptions for invalid inputs
  • Add more tests for invalid inputs

Related to #949.

Reminders

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to docstrings or tutorials.

Slash Commands

You can write slash commands (/command) in the first line of a comment to perform
specific operations. Supported slash commands are:

  • /format: automatically format and lint the code
  • /test-gmt-dev: run full tests on the latest GMT development version

@seisman seisman changed the title WIP: Refactor text to use virtual_from_data Refactor text to use virtual_from_data Mar 25, 2021
@seisman seisman marked this pull request as ready for review March 25, 2021 06:39
@seisman seisman changed the title Refactor text to use virtual_from_data Refactor text to use virtualfile_from_data Mar 25, 2021
@seisman seisman added the maintenance Boring but important stuff for the core devs label Mar 26, 2021
@seisman seisman added this to the 0.4.0 milestone Mar 26, 2021
@seisman seisman self-assigned this Mar 26, 2021
@seisman seisman mentioned this pull request Apr 5, 2021
Merged
5 tasks
maxrjones
maxrjones approved these changes Apr 15, 2021
View reviewed changes
Copy link
Member

@maxrjones maxrjones left a comment

Choose a reason for hiding this comment

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

Looks good to me. I only had a couple small comments.

pygmt/src/text.py Outdated
@@ -203,19 +204,15 @@ def text_(
extra_arrays.append(kwargs["t"])
kwargs["t"] = ""

# Append text as the last column
# text must be in str type, see issue #706
Copy link
Member

Choose a reason for hiding this comment

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

Should be fixed now, correct?

Suggested change
# text must be in str type, see issue #706

Copy link
Member Author

Choose a reason for hiding this comment

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

text must be in str type

This statement is still True.

pygmt/src/text.py Show resolved Hide resolved
pygmt/src/text.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented Apr 15, 2021

Thanks for your suggestions. I'll mark this PR as draft, because I don't like the current implemention.

@seisman seisman marked this pull request as draft April 15, 2021 14:52
@maxrjones maxrjones self-requested a review April 15, 2021 14:54
@core-man core-man mentioned this pull request May 20, 2021
Merged
5 tasks
@weiji14 weiji14 mentioned this pull request Jun 1, 2021
Closed
6 tasks
@weiji14 weiji14 modified the milestones: 0.4.0, 0.5.0 Jun 18, 2021
@weiji14 weiji14 mentioned this pull request Jul 23, 2021
Open
@weiji14 weiji14 mentioned this pull request Sep 1, 2021
Closed
@weiji14 weiji14 removed this from the 0.5.0 milestone Sep 30, 2021
@seisman seisman added this to the 0.7.0 milestone Mar 18, 2022
seisman
seisman commented May 14, 2022
View reviewed changes
pygmt/src/text.py Outdated Show resolved Hide resolved
@seisman seisman marked this pull request as ready for review May 14, 2022 10:33
weiji14
weiji14 reviewed May 16, 2022
View reviewed changes
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Had a quick glance and can't see any major issues with this refactor. Is there anything else remaining you want to work on here @seisman?

pygmt/src/text.py Outdated Show resolved Hide resolved
pygmt/src/text.py Outdated Show resolved Hide resolved
@seisman
Copy link
Member Author

seisman commented May 16, 2022

Is there anything else remaining you want to work on here?

I plan to not make more reactors in this PR to make it easier to review.

weiji14
weiji14 reviewed May 16, 2022
View reviewed changes
Copy link
Member

@weiji14 weiji14 left a comment

Choose a reason for hiding this comment

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

Looks ok to me. Sugest @meghanrjones (or someone else) to give this another review since it's been over a year since they approved this PR!

@weiji14 weiji14 added the final review call This PR requires final review and approval from a second reviewer label May 16, 2022
@seisman
Copy link
Member Author

seisman commented May 19, 2022

Ping @meghanrjones to give this PR a final review.

@seisman seisman removed the final review call This PR requires final review and approval from a second reviewer label May 22, 2022
@seisman seisman merged commit ed591a3 into main May 22, 2022
@seisman seisman deleted the refactor-text branch May 22, 2022 06:25
@maxrjones
Copy link
Member

Sorry for not getting back to this in time to be helpful

@seisman
Copy link
Member Author

seisman commented May 22, 2022

No worries. You can still leave your comments if there are any 😄

sixy6e pushed a commit to sixy6e/pygmt that referenced this pull request Dec 21, 2022
@seisman @maxrjones @weiji14
Refactor text to use virtualfile_from_data (GenericMappingTools#1121)
b1b408f 
Co-authored-by: Max Jones <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weiji14 weiji14 weiji14 left review comments

@core-man core-man core-man left review comments

@maxrjones maxrjones Awaiting requested review from maxrjones

Assignees

@seisman seisman

Labels
maintenance Boring but important stuff for the core devs
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@seisman @maxrjones @weiji14 @core-man