Repository navigation
Fix some linting issues in the template - #15
Conversation
|
Hi, I advise looking at source code comments and resolve spelling errors such as:
Also, it is not "commend" -> it is actually "comment". At least the add-ons are built correctly with this PR applied. A few things to consider, all in build vars:
I assume you did build several add-ons with this PR applied and all were successful (that will be considered a usability test). As for a space between the left and right sides of a colon (:), I should have caught that in summer - thanks for doing that. I'll merge this provided that you are willing to resolve typos. Thanks. |
|
Hi, If approved, it'll go into 2021.1 edition of Add-on Template at the latest (the earliest is October when 2020.3 edition will be published). Thanks. |
Restore 'import os.path' to reduce confusion and to make the template backward compatible.
|
And now, it will be merged - corrected spelling mistakes and restored os.path import statement with a Flake8 specific NOQA flag specified. As for linting pull requests, although NVDA Core config can serve as a starting point, be sure to ask authors about specifics if things are not clear. Going forward, I will be adding flake8.ini file to all my add-on repos because there are specifics I need to deal with that are not included in NVDA Core flake8.ini such as excluding third-party dependencies. Thanks. |
|
And now merged. |
|
@josephsl From your earlier comments I've assumed that there is no particular rush and decided to delay applying review action during the weekend when I'll have more time - seems for whatever reason you were unable to wait. What wories me is not the fact that you've decided to fix these problems yourself, though I think this should be left to the PR author, but the fact that in the next commits you've reverted some of my fixes. The main purpose of the Linter in my opinion is to encourage writing more optimized and cleaner code and my PR tried (without success since you've decided that you know better) to encourage add-on authors to do this.
|
|
Hi, it wasn’t my way, actually – the original author of Add-on Template is Rui Batista and Mesar Hameed, so I followed their comments and guidance to begin with. I apologize for closing this PR early without waiting. I also should have flagged lambda assignment source code comment also to reduce confusion from all sides. Thanks.
From: Łukasz Golonka <notifications@github.com>
Sent: Saturday, September 26, 2020 3:41 AM
To: nvdaaddons/AddonTemplate <AddonTemplate@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvdaaddons/AddonTemplate] Fix some linting issues in the template (#15)
@josephsl <https://github.com/josephsl> From your earlier comments I've assumed that there is no particular rush and decided to delay applying review action during the weekend when I'll have more time - seems for whatever reason you were unable to wait. What wories me is not the fact that you've decided to fix these problems yourself, though I think this should be left to the PR author, but the fact that in the next commits you've reverted some of my fixes. The main purpose of the Linter in my opinion is to encourage writing more optimized and cleaner code and my PR tried (without success since you've decided that you know better) to encourage add-on authors to do this.
In particular:
* Assigning lambda to the variables is discouraged by PEP8 not just because someone decided that he/she don't like this style but it really improves quality of tracebacks. What I did in <63e1ace> 63e1ace is syntactically the same as the previous lambda but encourages better coding style. Even if for this simple function it does not matter which I agree it does not developers can look at this and can have an example which is considered better by the Python community.
* Removal of os.path again people who use it to specify their Python sources do not understand that it's not needed and the env.Glob from scons can achieve exactly the same effect. I was going to add some explanatory comments about that but was not given a chance since again you decided that its better to do it your way rather than wait a while for other feedback.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#15 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4AXEH622ONWT5TZVLGZS3SHXAMBANCNFSM4RXMD75Q> .
|
|
Hi, The following actions were taken:
I guess the last thing to do is provide an explanation on glob expansion. I'm happy to take that as a follow-up pull request in addition to any SConstruct lint plan you have (although I must advise against fixing F821 as SConstruct variables and such come from SCons). Thanks. |
|
Hello @josephsl Sorry if my earlier comment sounded a little harsh - my only intention was to have a discussion about what coding style is better rather than blindly following comments like "Do not modify!". I'll submit second PR which would do the following:
With regard to F821 warnings they are fixable quite easily as follows. Since these names comes from SCons and are available by default the only problematic part is how to let Flake8 know where they are coming from which can be accomplished with the following at the top of the Sconstruct: Note that since these names are present already there should be no performance difference it is just to satisfy the Linter. |
|
Hi, note that we do need to test SCons scenario on multiple platforms (Windows, Linux, etc.), but I would imagine the solution will work. Thanks for the follow-up.
From: Łukasz Golonka <notifications@github.com>
Sent: Sunday, September 27, 2020 4:23 AM
To: nvdaaddons/AddonTemplate <AddonTemplate@noreply.github.com>
Cc: Joseph Lee <joseph.lee22590@gmail.com>; Mention <mention@noreply.github.com>
Subject: Re: [nvdaaddons/AddonTemplate] Fix some linting issues in the template (#15)
Hello @josephsl <https://github.com/josephsl> Sorry if my earlier comment sounded a little harsh - my only intention was to have a discussion about what coding style is better rather than blindly following comments like "Do not modify!". I'll submit second PR which would do the following:
-- Explain why the "_" is defined in buildVars - I believe having an explanatory comment there would make this more understandable.
* Provide some examples of specifying source files using env.Glob.
With regard to F821 warnings they are fixable quite easily as follows. Since these names comes from SCons and are available by default the only problematic part is how to let Flake8 know where they are coming from which can be accomplished with the following at the top of the Sconstruct:
from SCons.Script import Copy, Variables, BoolVariable, Environment, Builder
Note that since these names are present already there should be no performance difference it is just to satisfy the Linter.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#15 (comment)> , or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB4AXEGG7JBMKLQY53FFBI3SH4N77ANCNFSM4RXMD75Q> .
|
…ries (NVDA 2026.2) (nvdaaddons#19) Closes nvdaaddons#15 This pull request add support for building custom speech pronunciation dictionaries via the add-on template. Changes made: Adds "speechDictionraies" sectoin to buildVars Adds speech dictionaries manifest key type and generation rules (borrowing custom symbol dictionaries)
Normally I would refrain from submitting pull requests which contains just Linter fixes, but many recent commits to master are dealing with issues detected by Linter.
This PR makes buildVars, gettexttool and most of the sconstruct compliant with Flake8 rules (note that config from the NVDA repository was used when linting). If this would be accepted I intend to submit second PR which would deal with remaining issues in Sconstruct.