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

scripts/update-image: fix lint errors #130

Merged
merged 1 commit into from
Jul 25, 2021

Conversation

64
Copy link
Member

@64 64 commented Jul 23, 2021

Fix lint errors according to pyright.

Possibly related to #103.

Copy link
Member

@ArsenArsen ArsenArsen left a comment

Choose a reason for hiding this comment

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

Furthermore, rather than these faulty lints we should consider applying flake8 and some plugins to all our Python code, to enforce PEP8 guidelines and some other QA (import order, complexity requirements).

Comment on lines -12 to +21
@attr.s
class MountInfo:
blockdev = attr.ib()
mount_using = attr.ib()
image = attr.ib()
mountpoint = attr.ib()
root_idx = attr.ib()
efi_idx = attr.ib()
root_uuid = attr.ib()

@attr.s
def __init__(self, blockdev, mount_using, image, mountpoint, root_idx, efi_idx, root_uuid):
self.blockdev = blockdev
self.mount_using = mount_using
self.image = image
self.mountpoint = mountpoint
self.root_idx = root_idx
self.efi_idx = efi_idx
self.root_uuid = root_uuid

Copy link
Member

@ArsenArsen ArsenArsen Jul 24, 2021

Choose a reason for hiding this comment

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

No, attr.ib is used for a reason.

https://www.attrs.org/en/stable/

Copy link
Member Author

@64 64 Jul 24, 2021

Choose a reason for hiding this comment

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

I get that. But AFAICT the only feature we use is __init__. So I've just removed attrs entirely from this script.

Comment on lines 22 to +26
class Partition:
idx = attr.ib()
type = attr.ib()
uuid = attr.ib()
def __init__(self, idx, type, uuid):
self.idx = idx
self.type = type
self.uuid = uuid
Copy link
Member

Choose a reason for hiding this comment

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

Likewise (is this missing an @attr.s?)

@@ -299,9 +300,9 @@ def run(self):
# Determine root uuid manually for mountpoint
diskdev, partdev = dev_for_mountpoint(self.mountpoint)

if os.isdir('/dev/disk/by-uuid'):
if os.path.isdir('/dev/disk/by-uuid'):
Copy link
Member

Choose a reason for hiding this comment

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

import os.path as path is a common convention.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can replace all instances of os.path -> path, if that's what you're suggesting?

@64
Copy link
Member Author

64 commented Jul 24, 2021

I agree, we should really use PEP8.

@avdgrinten
Copy link
Member

I'm fine with keeping os.path. I'm indifferent about attr.s; if something can be achieved without an external lib, that's probably better anyway. @ArsenArsen what is the real advantage of attr.s here, other than not having to write the constructor?

@ArsenArsen
Copy link
Member

Here? Nothing specifically, this just seemed like a naive lint. If the dep was there already, and it'd appear that it was, it's probably fine to keep it, though.

As for path, I'm just mentioning a widespread convention.

@avdgrinten
Copy link
Member

Why did the linter complain about attr.s?

@ArsenArsen
Copy link
Member

Don't know, I'm just going off the initial comment:

Fix lint errors according to pyright.

@64
Copy link
Member Author

64 commented Jul 25, 2021

Why did the linter complain about attr.s?

To be clear: it's definitely a false positive lint. The decorator seems to confuse it. There's nothing actually broken about attrs here, unlike the other lint errors.

@avdgrinten
Copy link
Member

Alright, let's merge this now since there are no hard issues.

@avdgrinten avdgrinten merged commit 1a1503d into managarm:master Jul 25, 2021
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