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

tox: bump the pinned flake8 and pylint version (SC-422) #1029

Merged
merged 4 commits into from
Sep 21, 2021

Conversation

paride
Copy link
Contributor

@paride paride commented Sep 21, 2021

Proposed Commit Message

tox: bump the pinned flake8 and pylint version

* pylint: fix W1406 (redundant-u-string-prefix)

The u prefix for strings is no longer necessary in Python >=3.0.

* pylint: disable W1514 (unspecified-encoding)
    
From https://www.python.org/dev/peps/pep-0597/ (Python 3.10):
    
The new warning stems form https://www.python.org/dev/peps/pep-0597,
which says:
    
  Developers using macOS or Linux may forget that the default encoding
  is not always UTF-8. [...] Even Python experts may assume that the
  default encoding is UTF-8. This creates bugs that only happen on Windows.
    
The warning could be fixed by always specifying encoding='utf-8',
however we should be careful to not break environments which are not
utf-8 (or explicitly state that only utf-8 is supported). Let's silence
the warning for now.

* _quick_read_instance_id: cover the case where load_yaml() returns None

Spotted by pylint:
 - E1135 (unsupported-membership-test)
 - E1136 (unsubscriptable-object)

LP: #1944414

Additional Context

Spotted by the cloud-init-style-tip Jenkins job.

Test Steps

Reproduce with: tox -e tip-pylint. Test this branch by just running tox from it.

Checklist:

  • My code follows the process laid out in the documentation
  • I have updated or added any unit tests accordingly
  • I have updated or added any documentation accordingly

The u prefix for strings is no longer necessary in Python >=3.0.
@paride
Copy link
Contributor Author

paride commented Sep 21, 2021

This is a WIP because I'm not sure on how to fix this:

************* Module cloudinit.sources.DataSourceNoCloud
cloudinit/sources/DataSourceNoCloud.py:250: [E1135(unsupported-membership-test), _quick_read_instance_id] Value 'md' doesn't support membership test
cloudinit/sources/DataSourceNoCloud.py:251: [E1136(unsubscriptable-object), _quick_read_instance_id] Value 'md' is unsubscriptable

This happens here:


and the reason is that load_yaml() can return None, which of course has no membership test and isn't subscriptable:
def load_yaml(blob, default=None, allowed=(dict,)):

One way to fix this by applying this change to

-            if iid_key in md:
+            if md and iid_key in md:
                 return md[iid_key]

And this works, however I wonder if we should make load_yaml() output an empty dict by default instead of None. While it seems a good idea, it may be not because yaml.safe_load("") returns None and we may want to mimic that...

@paride paride changed the title [WIP] tox: bump the pinned flake8 and pylint version [WIP] tox: bump the pinned flake8 and pylint version (SC-422) Sep 21, 2021
@TheRealFalcon
Copy link
Member

TheRealFalcon commented Sep 21, 2021

I don't think this encoding change is safe to do as a bulk change. I'm guessing that most of these calls will work fine specifying utf-8, but are we sure that every single open is safe?

If I've specified my locale to use ISO-8859, are we sure that none of the files that we're reading will be encoded with ISO-8859 rather than utf-8? If we need to be explicit, then I think locale.getpreferredencoding(False) is the only safe option unless we're sure about every single open call.

For the yaml question, given that our load yaml definition is

def load_yaml(blob, default=None, allowed=(dict,)):

I think it makes sense to keep the default as None and do None checks for what has been returned.

@paride
Copy link
Contributor Author

paride commented Sep 21, 2021

I see your point and I'm find dropping the encoding= bulk commit and disable W1514 in pylintrc. We'll have to deal with this at some point, but with more care. +1 on keeping load_yaml() current behavior.

@paride paride changed the title [WIP] tox: bump the pinned flake8 and pylint version (SC-422) tox: bump the pinned flake8 and pylint version (SC-422) Sep 21, 2021
The new warning stems form https://www.python.org/dev/peps/pep-0597,
which says:

  Developers using macOS or Linux may forget that the default encoding
  is not always UTF-8. [...] Even Python experts may assume that the
  default encoding is UTF-8. This creates bugs that only happen on Windows.

The warning could be fixed by always specifying encoding='utf-8',
however we should be careful to not break environments which are not
utf-8 (or explicitly state that only utf-8 is supported). Let's silence
the warning for now.
Spotted by pylint:
 - E1135 (unsupported-membership-test)
 - E1136 (unsubscriptable-object)
Copy link
Member

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM assuming CI passes

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.

2 participants