Skip to content

Refactor Session._initialparts to have a more explicit type #6547

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

Merged
merged 1 commit into from
Jan 25, 2020

Conversation

bluetech
Copy link
Member

Previously, _initialparts was a list whose first item was a
py.path.local and the rest were strs. This is not something that
mypy is capable of modeling. The type List[Union[str, py.path.local]]
is too broad and would require asserts for every access.

Instead, make each item a Tuple[py.path.local, List[str]]. This way
the structure is clear and the types are accurate.

Copy link
Contributor

@blueyed blueyed left a comment

Choose a reason for hiding this comment

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

👍

@bluetech
Copy link
Member Author

@blueyed I forgot to mention, I'm working on these types you suggest, but I decided not to mix code changes with typing changes, so that they can be considered separately. I will send the typing changes in a separate PR.

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

LGTM, including @blueyed's suggestion.

Perhaps we should target features instead though, even if this is renaming an internal attribute?

@bluetech bluetech force-pushed the session-initialparts branch from 7a3ec7d to 3f52f30 Compare January 23, 2020 14:20
@bluetech
Copy link
Member Author

Thanks for review @blueyed @nicoddemus.

Perhaps we should target features instead though, even if this is renaming an internal attribute?

I prefer features too because I don't think these changes need to go on patch releases. But @blueyed sends typing changes to master so I started following suit. I must say I am still confused by this 😄. I think I and @blueyed should be synchronized on this, WDYT @blueyed?

If I have time this weekend, I'll try to address your comments on going forward with the master/features workflow changes. Then we won't have this dilemma :)

@nicoddemus
Copy link
Member

I prefer features too because I don't think these changes need to go on patch releases. But @blueyed sends typing changes to master so I started following suit.

I think adding type hints to master is fine, but in this case we are actually changing the attribute, from a list to a list of tuples, so it affects third party code using it (even though it shouldn't be accessing it).

Now that I mention this, perhaps we should also rename it? I think it is good practice to rename an attribute when the underlying data changes like this one, because I think an AttributeError would be better than the other kinds of errors this could cause. If people agree, I would suggest rename it to _initial_parts then.

@bluetech
Copy link
Member Author

If people agree, I would suggest rename it to _initial_parts then.

Good idea, I'll do it.

I think adding type hints to master is fine, but in this case we are actually changing the attribute

The tricky part here is that I only make this change so that further typing changes can depend on it. So they will need to go to features. As I said I actually prefer it, but I'm not sure @blueyed does.

@bluetech bluetech force-pushed the session-initialparts branch from 3f52f30 to 8b9601c Compare January 23, 2020 16:22
@blueyed
Copy link
Contributor

blueyed commented Jan 23, 2020

Perhaps we should target features instead though, even if this is renaming an internal attribute?

I think we do not need to care about compatibility for internal attributes really, but I agree that this should be avoided in bugfix releases.

Currently this means that this should go into features, but given the situation described in #6387 (comment) I actually think that we're about to drop features soon, and consider master what features is currently for us, using release branches for bugfix releases.

If I have time this weekend, I'll try to address your comments on going forward with the master/features workflow changes. Then we won't have this dilemma :)

That would be great. I've also started looking at the documented release process briefly (#6549). In my opinion we could start with a release-5.3 branch based on ddfa41b (the merge of 5.3.4). We should probably continue the discussion in #6387, and/or create a new issue to discuss/outline the process. This could also be done in #6549 maybe.

Anyway, for this PR I think it is better to stay with master, which shouldn't really be a problem when we're creating the next 5.3.x release as suggested above (and like I've suggested for 5.3.4 already).

@nicoddemus
Copy link
Member

using release branches for bugfix releases.

This is new to me... I thought we would just release from master every time, and what would decide if it was a bugfix release or a feature release would be the presence of new features on the branch, by looking at the changelog.

Or did you mean hotfixes?

(Anyway we should probably discuss this in another venue, sorry for the noise).

@nicoddemus
Copy link
Member

I think we do not need to care about compatibility for internal attributes really, but I agree that this should be avoided in bugfix releases.

Yes that's the point. 👍

Even if a plugin is wrongly accessing it, I would not like to "break" it for users in a bugfix release.

@bluetech bluetech force-pushed the session-initialparts branch from 8b9601c to 3286a6e Compare January 25, 2020 11:57
@bluetech bluetech changed the base branch from master to features January 25, 2020 11:57
Previously, _initialparts was a list whose first item was a
`py.path.local` and the rest were `str`s. This is not something that
mypy is capable of modeling. The type `List[Union[str, py.path.local]]`
is too broad and would require asserts for every access.

Instead, make each item a `Tuple[py.path.local, List[str]]`. This way
the structure is clear and the types are accurate.

To make sure any users who might have been accessing this (private)
field will not break silently, change the name to _initial_parts.
@bluetech bluetech force-pushed the session-initialparts branch from 3286a6e to dd5c2b2 Compare January 25, 2020 11:57
@bluetech
Copy link
Member Author

Rebased on features.

@bluetech bluetech merged commit a76bc64 into pytest-dev:features Jan 25, 2020
@bluetech bluetech deleted the session-initialparts branch January 25, 2020 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants