-
Notifications
You must be signed in to change notification settings - Fork 671
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
Core: add docstrings for launcher components #4148
base: main
Are you sure you want to change the base?
Conversation
worlds/LauncherComponents.py
Outdated
type: Type | ||
"""Classification of component intent to filter in the Launcher GUI""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little confused by what this is actually supposed to mean. Looking at nearby code helps a little, but after reading this, I'm not fully sure what type DOES, if I should set it, what I should set it to/with, and what happens if I choose one option over the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It affects the column and sort order of the Launcher UI.
Right side is Clients, left side is Tools, Misc, and Adjusters in that order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess my point is that it's not obvious when reading the docstring. At the least, I would expect that making a component a client puts it on the right side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
part of my intent on this wording is to be generic enough that it doesn't need to be rewritten with #3934
is the typing of type being Type the confusing part? If it was more obvious that there was an enum to reference about potential values help clarify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also when writing it I forgot that the init parameter for type is optional (and gets inferred by substrings of component name), because the actual value on the class is not optional
so that clarification could be expanded upon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referencing the enum could help
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with a bit more clarifications
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all sounds good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In light of #3934, code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reaffirming approval
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Little things
frozen_name: Optional[str] | ||
"""Recommended to use func instead; Name of the frozen EXE file for this component""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is EXE the right term here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not technically correct on linux i believe, but succinct enough for my tastes (willing to take suggestions)
Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What is this fixing or adding?
adds docstrings, mostly off the top of my head with some double-checking,
any wording suggestions and/or clarifications appreciated
How was this tested?
wasn't
If this makes graphical changes, please attach screenshots.