Skip to content

Conversation

@seisman
Copy link
Member

@seisman seisman commented Mar 25, 2021

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
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.

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.

@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 added this to the 0.7.0 milestone Mar 18, 2022
seisman and others added 2 commits May 14, 2022 18:10
Co-authored-by: Max Jones <meghanj@alum.mit.edu>
@seisman seisman marked this pull request as ready for review May 14, 2022 10:33
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?

@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.

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
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
@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
Co-authored-by: Max Jones <meghanj@alum.mit.edu>
Co-authored-by: Wei Ji <23487320+weiji14@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Boring but important stuff for the core devs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants