[ Feature ] Multi Tab - License Tab #246
Conversation
for more information, see https://pre-commit.ci
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| elif self.url.lower().endswith(".html"): | ||
| self.readme_data_type = ReadmeDataType.Html | ||
|
|
||
| with open(self.url, "r") as fd: |
Check warning
Code scanning / CodeQL
File is not always closed Warning
chennes
left a comment
chennes
left a comment
There was a problem hiding this comment.
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.
| def loadLicense(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.
That's a lot of whitespace here, for no benefit that I can see.
There was a problem hiding this comment.
Same reason as using mutliline or names other than
x , 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.
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 the loadLicense method (which PEP8 says should be called load_license).
There was a problem hiding this comment.
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.
for more information, see https://pre-commit.ci
…er into Feature-Tabs
for more information, see https://pre-commit.ci
| 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.
@FreeCAD/design-working-group what's our canonical capitalization of "README"?
There was a problem hiding this comment.
Doesn't seem like we will get any more comments any time soon, ..
I adjusted it to Overview + License.
|
I'll give this a refactoring pass today or tomorrow -- we shouldn't use a |
2 participants
Inspired by the work done here
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.