- Notifications
You must be signed in to change notification settings - Fork15
[ Feature ] Multi Tab - License Tab#246
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:dev
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| elifself.url.lower().endswith(".html"): | ||
| self.readme_data_type=ReadmeDataType.Html | ||
| withopen(self.url,"r")asfd: |
Check warning
Code scanning / CodeQL
File is not always closed Warning
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 my fault, seize!
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 few minor comments, below. Also your new code includes blank lines between nearly every line of code, so I don't know if your IDE doesn't like something or what.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| defloadLicense(self,license:License,text:QtWidgets.QTextEdit): | ||
| url=utils.construct_git_url(self.addon,license.file) | ||
| manager=NetworkManager.AM_NETWORK_MANAGER | ||
| index=manager.submit_unmonitored_get(url) | ||
| self.license_requests[index]= {"license":license,"text":text} | ||
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.
That's a lot of whitespace here, for no benefit that I can see.
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.
Same reason as using mutliline or names other thanx ,y ,z for variable names, makes it easier to read?
Group together what belongs together..
You have auto-formatting rules, if you don't
like it, shouldn't you have configured that?
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.
The rule with using Black is, you don't configure Black 😁 -- we break that by setting the line length, but otherwise there aren't any settings. Black will let you put individual blank lines in as you see fit, which is good because it's basically impossible to check that programmatically. I'm just questioning these blank lines: there's nothing to "group", you have four individual lines. So the only grouping is that they are all part of theloadLicense method (which PEP8 says should be calledload_license).
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.
Well yeah, each line does something different, if I had something like x = y() and then a bunch of indexing into x like x.z = '' I'd put them together.
I have a hard time reading blocks of text so I space things 0 , 1 , 2 lines apart depending on the context.
I can smush them all together if you'd like.
Uh oh!
There was an error while loading.Please reload this page.
for more information, seehttps://pre-commit.ci
…er into Feature-Tabs
for more information, seehttps://pre-commit.ci
Uh oh!
There was an error while loading.Please reload this page.
| self.tabs_widget=QtWidgets.QTabWidget(self) | ||
| self.tabs[TabView.Readme]=self.tabs_widget.addTab( | ||
| self.readme_browser,translate("AddonsInstaller","README") |
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.
@FreeCAD/design-working-group what's our canonical capitalization of "README"?
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.
Doesn't seem like we will get any more comments any time soon, ..
I adjusted it toOverview +License.
I'll give this a refactoring pass today or tomorrow -- we shouldn't use a |
Inspired by the work donehere
I decided to go with a minimal
approach for my own implementation.
LicensetabHelp wanted, if someone with Python async knowledge
could make this not block the UI, that's be just great ..
Implementation Notes
I decided to have mapping of the tabs to their tab indices
to not have to hard code them and keep them in sync.
The license text is currently just dumped in the
tab, for now I just wanted to get it to work.