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.

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

refactor: cobra.io.yaml #1152

Merged
merged 3 commits into from
Feb 18, 2022
Merged

refactor: cobra.io.yaml #1152

merged 3 commits into from
Feb 18, 2022

Conversation

synchon
Copy link
Member

@synchon synchon commented Feb 17, 2022

  • fix #(issue number)
  • description of feature/fix
  • tests added/passed
  • add an entry to the next release

This PR adds typing annotations for cobra.io.yaml and its unit tests. It also upgrades the code style for them to maintain Python 3.6+ compatibility.

@synchon synchon self-assigned this Feb 17, 2022
@synchon synchon added the ready Finished PR that requires review and merge. label Feb 17, 2022
class CobraYAML(YAML):
"""Define custom subclass for YAML I/O."""

def dump(self, data: Dict, stream: Optional[Any] = None, **kwargs: Any) -> str:
Copy link
Member

Choose a reason for hiding this comment

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

I think the type of stream can be https://docs.python.org/3/library/io.html#io.TextIOBase rather than Any.

Copy link
Member Author

Choose a reason for hiding this comment

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

What about we use something more specific: https://docs.python.org/3/library/io.html#io.StringIO ?

Copy link
Member

Choose a reason for hiding this comment

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

My thinking was that if someone passes a stream, it will not be StringIO but some other text stream. So it's a bit more broad to go with TextIOBase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay sounds fair. I'll change it.

Copy link
Member

@Midnighter Midnighter left a comment

Choose a reason for hiding this comment

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

Thank you 🙂

@synchon synchon merged commit 879607c into devel Feb 18, 2022
@synchon synchon deleted the refactor/io-yaml branch February 18, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ready Finished PR that requires review and merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants