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

Move RichGroup and RichCommand out of rich_click.py #37

Merged
merged 2 commits into from Mar 7, 2022

Conversation

Jorricks
Copy link
Contributor

@Jorricks Jorricks commented Mar 7, 2022

First of a couple refactorings that are coming up.
This refactoring:

  • Removes local imports. It's generally more preferred to use absolute imports.
  • I moved the classes out of rich_click.py. This made the rich_click.py unnecessarily large..
  • Moved the creation of the console into a function, as it's used all over the place.

I think to tune this file down more, we might consider moving all rich specific tools to a file called rich_utils and moving the click items into click_utils.

@Jorricks
Copy link
Contributor Author

Jorricks commented Mar 7, 2022

Other things I was thinking would be a good idea:

  1. Add typing info to each argument of a function
  2. Introduce pre-commit and thereby: black, flake8 and pytype.
    What do you think @ewels?

Copy link
Owner

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Yes, much better - thanks 👍🏻 The console thing had been in the back of my mind for a bit as well.

I wonder if we can just create a single top-level global console object and then use that everywhere? (Similar to the highlighter). I'm not sure that there's any need to create so many different versions of the same thing. Might be cleaner.

@ewels
Copy link
Owner

ewels commented Mar 7, 2022

Other things I was thinking would be a good idea:

  1. Add typing info to each argument of a function
  2. Introduce pre-commit and thereby: black, flake8 and pytype.
    What do you think @ewels?

Yes to all of this 👍🏻 I'm a scientist first and coder second and have never gotten around to properly teaching myself to use typing. But I've been feeling increasingly guilty about it! If you fancy adding typing to this then that would be great actually, as it'll force me to use it for any new features I add 😅

Pre-commit stuff also good. Again I've never bothered setting this up myself (instead relying on VSCode plugins) but I should probably set myself up to use it.

@ewels
Copy link
Owner

ewels commented Mar 7, 2022

See also #12 but that lower priority stuff that can probably be handled in a separate PR

@Jorricks
Copy link
Contributor Author

Jorricks commented Mar 7, 2022

Yes, much better - thanks 👍🏻 The console thing had been in the back of my mind for a bit as well.

I wonder if we can just create a single top-level global console object and then use that everywhere? (Similar to the highlighter). I'm not sure that there's any need to create so many different versions of the same thing. Might be cleaner.

I don't like global variables to be fair.
I think for the way you are using them for settings, it's quite nice, but in general for things like getting a console, I rather use functions. I usually achieve the same behaviour as a global variable by using the functools.lru_cache decorator over functions.
I think it would fit well in this use-case as well.

@Jorricks
Copy link
Contributor Author

Jorricks commented Mar 7, 2022

Fixed black formatting 👍 .
Cool I'll proceed with the other PRs once this one is concluded ✌️

@ewels
Copy link
Owner

ewels commented Mar 7, 2022

I think it would fit well in this use-case as well.

Sounds good 👍🏻 I'm not familiar with it but I'm happy to defer to you.. I agree about generally disliking globals for this kind of thing, I was mostly thinking about consistency as I saw the highlighter line just above the console function. Maybe we can do something similar for both.

Copy link
Owner

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Changelog entries would be good, but as you're going to have a follow-on PR can do that later.

@ewels ewels merged commit daf037a into ewels:main Mar 7, 2022
@Jorricks Jorricks deleted the move-classes-out branch March 7, 2022 12:10
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.

None yet

2 participants