Handle the last of the type ignore

[Peilonrayz]

1. Simple changes

With the exception of a couple of # type: ignores that can't really be removed. I would like to remove all that are left. I have had a quick glance at the ones that remain after #290, and to remove them would require:

• Use --ignore-missing-imports to remove all ignores around imports.

• Change _parametrize.parametrize_decorator to delegate the transformation of arg_values_list to a function so we don't reuse the same variable name.
  Alternately we could create a new variable and change the two ifs to an if elif else.

• Change sessions.Session.virtualenv to treat None as an AttributeError, changing the return type to ProcessEnv rather than Optional[ProcessEnv].

  Additionally change all instances of self._runner.venv to self.virtualenv. This would require some changes to the tests.

• Change sessions.Session.error and skip to not allow keyword arguments. As they just produce the following TypeError:

  TypeError: Exception() takes no keyword arguments

• Add a create method to virtualenv.ProcessEnv that just raises NotImplementedException. This is because we're using PrecessEnv like an interface, as the two subtypes CondaEnv and VirtualEnv both define this method.

• Change tox_to_nox to raise an error if pkgutil.get_data returns None.
  Alternately we could just continue ignoring this.

These are all quite small changes, I can't imagine they would take more than a couple lines of code to fix.

2. Complex changes

There are some more complex changes I'd like to make. Any that change what the type annotations are can have the ability to effect large amounts of the code.

• I think changing nox.manifest.KeywordsLocal to inherit from collections.abc.Mapping would allow us to remove the # type: ignore.
• Change annotations to be correct. We are passing things like Optional[str] to a function that only takes str.
• Change sessions.Session.notify to error if manifest is None. Or
  Change sessions.SessionRunner to not allow None as a value to manifest.

To solve the problematic type annotations should be simple, change the annotation to Optional[str] and handle None further down the code. If this isn't clean then I can handle None by throwing an error before entering the function/class/method.

---

If you say the above is all good then I'll implement the first solution I list on any where I list multiple, such as 1.2. If you think I've missed the mark I'm happy to fix these any other way.

I'm new to GitHub, are the current size of the PRs ok? Should I try split this one up so it's multiple smaller ones, or is just one big one ok?

[theacodes]

Go for it. The last PR was about at my limit in terms of size. Always happy to merge several smaller PRs that work towards the same goal.

…

On Thu, Mar 5, 2020, 7:08 PM Peilonrayz ***@***.***> wrote: 1. Simple changes With the exception of a couple of # type: ignores that can't really be removed. I would like to remove all that are left. I have had a quick glance at the ones that remain after #290 <#290>, and to remove them would require: 1. Use --ignore-missing-imports <https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-ignore-missing-imports> to remove all ignores around imports. 2. Change _parametrize.parametrize_decorator <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L123> to delegate the transformation of arg_values_list to a function so we don't reuse the same variable name. Alternately we could create a new variable and change the two ifs <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L125> to an if elif else. 3. Change sessions.Session.virtualenv <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L121> to treat None as an AttributeError, changing the return type to ProcessEnv rather than Optional[ProcessEnv]. Additionally change all instances of self._runner.venv to self.virtualenv. This would require some changes to the tests. 4. Change sessions.Session.error <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L346> and skip <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L350> to not allow keyword arguments. As they just produce the following TypeError: TypeError: Exception() takes no keyword arguments 5. Add a create method to virtualenv.ProcessEnv <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/virtualenv.py#L40> that just raises NotImplementedException. This is because we're using PrecessEnv like an interface, as the two subtypes CondaEnv and VirtualEnv both define this method. 6. Change tox_to_nox to raise an error if pkgutil.get_data returns None <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/tox_to_nox.py#L26> . Alternately we could just continue ignoring this. These are all quite small changes, I can't imagine they would take more than a couple lines of code to fix. 2. Complex changes There are some more complex changes I'd like to make. Any that change what the type annotations are can have the ability to effect large amounts of the code. 1. I think changing nox.manifest.KeywordsLocal <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/manifest.py#L246> to inherit from collections.abc.Mapping <https://docs.python.org/3/library/collections.abc.html#collections.abc.Mapping> would allow us to remove the # type: ignore. 2. Change annotations to be correct. We are passing things like Optional[str] to a function that only takes str. 3. Change sessions.Session.notify <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L329> to error if manifest is None. Or Change sessions.SessionRunner <https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L355> to not allow None as a value to manifest. To solve the problematic type annotations should be simple, change the annotation to Optional[str] and handle None further down the code. If this isn't clean then I can handle None by throwing an error before entering the function/class/method. ------------------------------ If you say the above is all good then I'll implement the first solution I list on any where I list multiple, such as 1.2. If you think I've missed the mark I'm happy to fix these any other way. I'm new to GitHub, are the current size of the PRs ok? Should I try split this one up so it's multiple smaller ones, or is just one big one ok? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#292>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I46MUS2GEOPKL4DJLSTRGBSJFANCNFSM4LCXSPFA> .

[Peilonrayz]

Sure thing. Ok, sorry. I'll try keep them on the smaller size from now on.

…

On Fri, 6 Mar 2020, 03:11 Thea Flowers, ***@***.***> wrote: Go for it. The last PR was about at my limit in terms of size. Always happy to merge several smaller PRs that work towards the same goal. On Thu, Mar 5, 2020, 7:08 PM Peilonrayz ***@***.***> wrote: > 1. Simple changes > > With the exception of a couple of # type: ignores that can't really be > removed. I would like to remove all that are left. I have had a quick > glance at the ones that remain after #290 > <#290>, and to remove them would > require: > > 1. > > Use --ignore-missing-imports > < https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-ignore-missing-imports > > to remove all ignores around imports. > 2. > > Change _parametrize.parametrize_decorator > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L123 > > to delegate the transformation of arg_values_list to a function so we > don't reuse the same variable name. > Alternately we could create a new variable and change the two ifs > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L125 > > to an if elif else. > 3. > > Change sessions.Session.virtualenv > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L121 > > to treat None as an AttributeError, changing the return type to > ProcessEnv rather than Optional[ProcessEnv]. > > Additionally change all instances of self._runner.venv to > self.virtualenv. This would require some changes to the tests. > 4. > > Change sessions.Session.error > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L346 > > and skip > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L350 > > to not allow keyword arguments. As they just produce the following > TypeError: > > TypeError: Exception() takes no keyword arguments > > 5. > > Add a create method to virtualenv.ProcessEnv > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/virtualenv.py#L40 > > that just raises NotImplementedException. This is because we're using > PrecessEnv like an interface, as the two subtypes CondaEnv and > VirtualEnv both define this method. > 6. > > Change tox_to_nox to raise an error if pkgutil.get_data returns None > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/tox_to_nox.py#L26 > > . > Alternately we could just continue ignoring this. > > These are all quite small changes, I can't imagine they would take more > than a couple lines of code to fix. > 2. Complex changes > > There are some more complex changes I'd like to make. Any that change what > the type annotations are can have the ability to effect large amounts of > the code. > > 1. I think changing nox.manifest.KeywordsLocal > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/manifest.py#L246 > > to inherit from collections.abc.Mapping > < https://docs.python.org/3/library/collections.abc.html#collections.abc.Mapping > > would allow us to remove the # type: ignore. > 2. Change annotations to be correct. We are passing things like > Optional[str] to a function that only takes str. > 3. Change sessions.Session.notify > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L329 > > to error if manifest is None. Or > Change sessions.SessionRunner > < https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L355 > > to not allow None as a value to manifest. > > To solve the problematic type annotations should be simple, change the > annotation to Optional[str] and handle None further down the code. If > this isn't clean then I can handle None by throwing an error before > entering the function/class/method. > ------------------------------ > > If you say the above is all good then I'll implement the first solution I > list on any where I list multiple, such as 1.2. If you think I've missed > the mark I'm happy to fix these any other way. > > I'm new to GitHub, are the current size of the PRs ok? Should I try split > this one up so it's multiple smaller ones, or is just one big one ok? > > — > You are receiving this because you are subscribed to this thread. > Reply to this email directly, view it on GitHub > < #292 >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/AAB5I46MUS2GEOPKL4DJLSTRGBSJFANCNFSM4LCXSPFA > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#292>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABK42NKACVGC4JGSVV74FW3RGBSWZANCNFSM4LCXSPFA> .

[theacodes]

Absolutely no need to apologize. I appreciate all of this work you've been contributing!

…

On Thu, Mar 5, 2020, 7:18 PM Peilonrayz ***@***.***> wrote: Sure thing. Ok, sorry. I'll try keep them on the smaller size from now on. On Fri, 6 Mar 2020, 03:11 Thea Flowers, ***@***.***> wrote: > Go for it. The last PR was about at my limit in terms of size. Always happy > to merge several smaller PRs that work towards the same goal. > > On Thu, Mar 5, 2020, 7:08 PM Peilonrayz ***@***.***> wrote: > > > 1. Simple changes > > > > With the exception of a couple of # type: ignores that can't really be > > removed. I would like to remove all that are left. I have had a quick > > glance at the ones that remain after #290 > > <#290>, and to remove them would > > require: > > > > 1. > > > > Use --ignore-missing-imports > > < > https://mypy.readthedocs.io/en/stable/command_line.html#cmdoption-mypy-ignore-missing-imports > > > > to remove all ignores around imports. > > 2. > > > > Change _parametrize.parametrize_decorator > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L123 > > > > to delegate the transformation of arg_values_list to a function so we > > don't reuse the same variable name. > > Alternately we could create a new variable and change the two ifs > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/_parametrize.py#L125 > > > > to an if elif else. > > 3. > > > > Change sessions.Session.virtualenv > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L121 > > > > to treat None as an AttributeError, changing the return type to > > ProcessEnv rather than Optional[ProcessEnv]. > > > > Additionally change all instances of self._runner.venv to > > self.virtualenv. This would require some changes to the tests. > > 4. > > > > Change sessions.Session.error > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L346 > > > > and skip > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L350 > > > > to not allow keyword arguments. As they just produce the following > > TypeError: > > > > TypeError: Exception() takes no keyword arguments > > > > 5. > > > > Add a create method to virtualenv.ProcessEnv > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/virtualenv.py#L40 > > > > that just raises NotImplementedException. This is because we're using > > PrecessEnv like an interface, as the two subtypes CondaEnv and > > VirtualEnv both define this method. > > 6. > > > > Change tox_to_nox to raise an error if pkgutil.get_data returns None > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/tox_to_nox.py#L26 > > > > . > > Alternately we could just continue ignoring this. > > > > These are all quite small changes, I can't imagine they would take more > > than a couple lines of code to fix. > > 2. Complex changes > > > > There are some more complex changes I'd like to make. Any that change > what > > the type annotations are can have the ability to effect large amounts of > > the code. > > > > 1. I think changing nox.manifest.KeywordsLocal > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/manifest.py#L246 > > > > to inherit from collections.abc.Mapping > > < > https://docs.python.org/3/library/collections.abc.html#collections.abc.Mapping > > > > would allow us to remove the # type: ignore. > > 2. Change annotations to be correct. We are passing things like > > Optional[str] to a function that only takes str. > > 3. Change sessions.Session.notify > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L329 > > > > to error if manifest is None. Or > > Change sessions.SessionRunner > > < > https://github.com/theacodes/nox/blob/8869bf2a20d51343e76459ca50ae2dc36d2b46e2/nox/sessions.py#L355 > > > > to not allow None as a value to manifest. > > > > To solve the problematic type annotations should be simple, change the > > annotation to Optional[str] and handle None further down the code. If > > this isn't clean then I can handle None by throwing an error before > > entering the function/class/method. > > ------------------------------ > > > > If you say the above is all good then I'll implement the first solution I > > list on any where I list multiple, such as 1.2. If you think I've missed > > the mark I'm happy to fix these any other way. > > > > I'm new to GitHub, are the current size of the PRs ok? Should I try split > > this one up so it's multiple smaller ones, or is just one big one ok? > > > > — > > You are receiving this because you are subscribed to this thread. > > Reply to this email directly, view it on GitHub > > < > #292 > >, > > or unsubscribe > > < > https://github.com/notifications/unsubscribe-auth/AAB5I46MUS2GEOPKL4DJLSTRGBSJFANCNFSM4LCXSPFA > > > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > < #292 >, > or unsubscribe > < https://github.com/notifications/unsubscribe-auth/ABK42NKACVGC4JGSVV74FW3RGBSWZANCNFSM4LCXSPFA > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#292>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAB5I42IDT2WCCB72DNAXQ3RGBTQZANCNFSM4LCXSPFA> .