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

Some cleanup of config.py #2

Merged
merged 2 commits into from
Mar 17, 2022
Merged

Some cleanup of config.py #2

merged 2 commits into from
Mar 17, 2022

Conversation

Dr-HyperCake
Copy link
Contributor

No description provided.

else:
if len(key_paths) > 1:
for path in key_paths:
basename = os.path.basename(path)
Copy link
Owner

Choose a reason for hiding this comment

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

this should be changed to basename = os.path.basename(path[0]) as the path that gets passed into there is already a tuple

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would get fixed if the other issue gets fixed, don't worry about it for now


if self.get_update_status() is None:
self.set_update(True)

if self.get_color() is None:
self.write_color('DarkBlue3')

def write_key_path(self, key_path: tuple):
def write_key_paths(self, *key_paths):
Copy link
Owner

Choose a reason for hiding this comment

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

should remove the * due to anything being passed into it is already a tuple by force.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I opted for an argument pack (as well as changing the name of the function) because the function takes variable number of arguments, "one or more key paths for the config to write". While it can be done the same with a single iterable argument, I thought this was a little bit cleaner. That said I am happy to revert this one if you prefer it without the pack. Do note though that in the absence of type-checking, the : tuple annotation doesn't actually force key_paths to be a tuple.

Copy link
Owner

Choose a reason for hiding this comment

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

i'm just saying that the only time we use that function is when we pass in a tuple, so the * doubles the tuple. it would be greatly appreciated if the * was removed

@jozz024 jozz024 requested a review from MiDe-S March 17, 2022 10:10
config.py Show resolved Hide resolved
@Dr-HyperCake
Copy link
Contributor Author

I had neglected to update main.py before, but I've now fixed both the name, and the issue with unpacking the tuple.

@jozz024 jozz024 merged commit fd4f4e4 into jozz024:main Mar 17, 2022
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