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

MicroSD.is_inserted as an unsettable attribute #446

Merged
merged 2 commits into from
Aug 23, 2023

Conversation

jdlcdl
Copy link

@jdlcdl jdlcdl commented Aug 19, 2023

This pr resolves an error-prone is_enabled() method call by accessing that name as an attribute unintended reference to a class attribute instead of calling the name as a class method effectively enabling user to pull the microsd before startup and allowing seedsigner to learn the microsd inserted/removed state.

@kdmukai
Copy link
Contributor

kdmukai commented Aug 19, 2023

Since the "is_" pattern is so ingrained as a property, I think the way is_inserted() is defined will continue to be error-prone.

We'd be better off either renaming the classmethod to a slightly different name that is more intuitively method-y (but I can't think of any such name) OR, even better, change it to be a @property:

# microsd.py
    ...
    @property
    def is_inserted(self):
        ...

# microsd.py MicroSD.run()
Settings.handle_microsd_state_change(
    action=MicroSD.ACTION__INSERTED if self.is_inserted else MicroSD.ACTION__REMOVED
)
# toast.py RemoveSDCardToastManagerThread.should_keep_running()
MicroSD.get_instance().is_inserted

@jdlcdl jdlcdl changed the title .is_inserted() as method at startup MicroSD.is_inserted as an unsettable attribute Aug 19, 2023
@newtonick newtonick added this to the 0.7.0 milestone Aug 21, 2023
@newtonick
Copy link
Collaborator

ACK and Tested

@newtonick newtonick merged commit 9451a9d into SeedSigner:dev Aug 23, 2023
@jdlcdl jdlcdl deleted the microsd_is_inserted_startup_fix branch September 16, 2023 12:24
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