Skip to content

minor modifications to Spec.__repr__#21723

Closed
cosmicexplorer wants to merge 1 commit intospack:developfrom
cosmicexplorer:fix-spec-str
Closed

minor modifications to Spec.__repr__#21723
cosmicexplorer wants to merge 1 commit intospack:developfrom
cosmicexplorer:fix-spec-str

Conversation

@cosmicexplorer
Copy link
Copy Markdown
Contributor

@cosmicexplorer cosmicexplorer commented Feb 17, 2021

This PR is composed of two minor changes to the Spec class, along with two further distinct changes which I would like some review on:

  1. Add type annotations to Spec.__init__().
  2. Correctly implement Spec.__repr__() to return a string which can be eval()ed to produce the same object.

Desired Feedback

  • Are these types correct?
  • Is this repr() impl more correct? Is there a specific reason we were breaking the repr() contract by calling str() before?

@cosmicexplorer cosmicexplorer changed the title add type annotations to Spec.__init__() and fix Spec.__repr__() minor modifications to the Spec class definition Feb 17, 2021
@cosmicexplorer cosmicexplorer changed the title minor modifications to the Spec class definition minor modifications to Spec, conftest.py, and SSL contexts Feb 17, 2021
Copy link
Copy Markdown
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

Let's get rid of the type stuff if it's not the minimally necessary set of type annotations for mypy to pass. As we discussed, we're not going all-in on typing until we can at least ditch Python 2 (which will be a while).

RE:

Is this repr() impl more correct? Is there a specific reason we were breaking the repr() contract by calling str() before?

I would need to look at this in more detail, but one thing to consider is that we don't always have a way to create a reproducible repr() for a concrete spec. That may take some more work to fix.

@cosmicexplorer cosmicexplorer changed the title minor modifications to Spec, conftest.py, and SSL contexts minor modifications to Spec.__repr__ Jan 16, 2022
@cosmicexplorer
Copy link
Copy Markdown
Contributor Author

There's no need for this PR any further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants