Python: Function/Class Naming Convention (Syntax)#3810
Conversation
RasmusWL
left a comment
There was a problem hiding this comment.
Thanks for your submission @dilanbhalla 👍
Overall I think these cases are better handled by a linter (and pylint does mark these with the invalid-name rule), but I also don't see anything wrong with adding these queries. There is a few minor changes I would like to see just quickly looking over the code.
@tausbn how do you feel about adding these queires?
|
Hello,
Thank you for the fixes. Looked great to me so I committed them, let me know if there is anything else you would like me to do! (I added these before I was informed that I should submit new queries under “experimental,” so if you would like me to move these there I can definitely do that as well)
Best,
Dilan
… On Jun 26, 2020, at 4:42 AM, Rasmus Wriedt Larsen ***@***.***> wrote:
@RasmusWL requested changes on this pull request.
Thanks for your submission @dilanbhalla 👍
Overall I think these cases are better handled by a linter (and pylint does mark these with the invalid-name rule), but I also don't see anything wrong with adding these queries. There is a few minor changes I would like to see just quickly looking over the code.
@tausbn how do you feel about adding these queires?
In python/ql/src/Functions/NamingConventionsFunctions.qhelp:
> @@ -0,0 +1,30 @@
+<!DOCTYPE qhelp PUBLIC
+ "-//Semmle//qhelp//EN"
+ "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>A function name that begins with an uppercase letter does not follow standard
+naming conventions. This decreases code readability. For example, <code>Jump</code>.
⬇️ Suggested change
-naming conventions. This decreases code readability. For example, <code>Jump</code>.
+naming conventions. This decreases code readability. For example, <code>def Jump():</code>.
In python/ql/src/Functions/NamingConventionsFunctions.qhelp:
> +<!DOCTYPE qhelp PUBLIC
+ "-//Semmle//qhelp//EN"
+ "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>A function name that begins with an uppercase letter does not follow standard
+naming conventions. This decreases code readability. For example, <code>Jump</code>.
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Write the function name beginning with an lowercase letter. For example, <code>jump</code>.
⬇️ Suggested change
-Write the function name beginning with an lowercase letter. For example, <code>jump</code>.
+Write the function name beginning with an lowercase letter. For example, <code>def jump():</code>.
In python/ql/src/Classes/NamingConventionsClasses.ql:
> @@ -0,0 +1,17 @@
+/**
+ * @name Misnamed class
+ * @description A class name that begins with a lowercase letter decreases readability.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision medium
+ * @id python/misnamed-type
+ * @tags maintainability
⬇️ Suggested change
- * @tags maintainability
+ * @tags readability
In python/ql/src/Functions/NamingConventionsFunctions.ql:
> @@ -0,0 +1,17 @@
+/**
+ * @name Misnamed function
+ * @description A function name that begins with an uppercase letter decreases readability.
+ * @kind problem
+ * @problem.severity recommendation
+ * @precision medium
+ * @id python/misnamed-function
+ * @tags maintainability
⬇️ Suggested change
- * @tags maintainability
+ * @tags readability
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
|
|
Hello,
I just moved this query to experimental in case it would like to be reviewed there first as well. Thank you again and let me know if I need to include anything else!
Best,
Dilan
… On Jul 1, 2020, at 9:34 AM, Dilan Bhalla ***@***.***> wrote:
Hello,
Thank you for the fixes. Looked great to me so I committed them, let me know if there is anything else you would like me to do! (I added these before I was informed that I should submit new queries under “experimental,” so if you would like me to move these there I can definitely do that as well)
Best,
Dilan
>> On Jun 26, 2020, at 4:42 AM, Rasmus Wriedt Larsen ***@***.***> wrote:
>>
>
> @RasmusWL requested changes on this pull request.
>
> Thanks for your submission @dilanbhalla 👍
>
> Overall I think these cases are better handled by a linter (and pylint does mark these with the invalid-name rule), but I also don't see anything wrong with adding these queries. There is a few minor changes I would like to see just quickly looking over the code.
>
> @tausbn how do you feel about adding these queires?
>
> In python/ql/src/Functions/NamingConventionsFunctions.qhelp:
>
> > @@ -0,0 +1,30 @@
> +<!DOCTYPE qhelp PUBLIC
> + "-//Semmle//qhelp//EN"
> + "qhelp.dtd">
> +<qhelp>
> +
> +
> +<overview>
> +<p>A function name that begins with an uppercase letter does not follow standard
> +naming conventions. This decreases code readability. For example, <code>Jump</code>.
> ⬇️ Suggested change
> -naming conventions. This decreases code readability. For example, <code>Jump</code>.
> +naming conventions. This decreases code readability. For example, <code>def Jump():</code>.
> In python/ql/src/Functions/NamingConventionsFunctions.qhelp:
>
> > +<!DOCTYPE qhelp PUBLIC
> + "-//Semmle//qhelp//EN"
> + "qhelp.dtd">
> +<qhelp>
> +
> +
> +<overview>
> +<p>A function name that begins with an uppercase letter does not follow standard
> +naming conventions. This decreases code readability. For example, <code>Jump</code>.
> +</p>
> +
> +</overview>
> +<recommendation>
> +
> +<p>
> +Write the function name beginning with an lowercase letter. For example, <code>jump</code>.
> ⬇️ Suggested change
> -Write the function name beginning with an lowercase letter. For example, <code>jump</code>.
> +Write the function name beginning with an lowercase letter. For example, <code>def jump():</code>.
> In python/ql/src/Classes/NamingConventionsClasses.ql:
>
> > @@ -0,0 +1,17 @@
> +/**
> + * @name Misnamed class
> + * @description A class name that begins with a lowercase letter decreases readability.
> + * @kind problem
> + * @problem.severity recommendation
> + * @precision medium
> + * @id python/misnamed-type
> + * @tags maintainability
> ⬇️ Suggested change
> - * @tags maintainability
> + * @tags readability
> In python/ql/src/Functions/NamingConventionsFunctions.ql:
>
> > @@ -0,0 +1,17 @@
> +/**
> + * @name Misnamed function
> + * @description A function name that begins with an uppercase letter decreases readability.
> + * @kind problem
> + * @problem.severity recommendation
> + * @precision medium
> + * @id python/misnamed-function
> + * @tags maintainability
> ⬇️ Suggested change
> - * @tags maintainability
> + * @tags readability
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub, or unsubscribe.
|
tausbn
left a comment
There was a problem hiding this comment.
Thank you for your contribution.
I ran these queries against a couple of large/popular projects here:
Class names: https://lgtm.com/query/6870760152721696664/
Function names: https://lgtm.com/query/1144809513175497672/
I sampled a random selection of results, but none of them looked like true positives to me. Here I mean "true positive" in the sense that this would be something a developer would be inclined to fix upon seeing the alert. From this point of view, these queries are much too noisy to run automatically (though they may be of use to people who wish to enforce coding standards).
One possible improvement would be to also look at the context in which these declarations appear. For instance, if all (or most) classes in a given file are "illegally" named, then probably this was intended. I would be curious to see how many false positives this would eliminate.
tausbn
left a comment
There was a problem hiding this comment.
Hello! Thank you for your patience. I have made a few suggestions, two for the .qhelp files which had incorrect closing tags, and two for the queries themselves. I think this looks pretty close to being mergeable.
(And as you can tell, we're slowly getting back to reviewing external contributions again. We'll be revisiting the rest of your contributions in the near future.)
|
Apologies for the delay, and thanks for all of your comments! I incorporated all of the fixes. |
tausbn
left a comment
There was a problem hiding this comment.
I think this looks good to merge. However, it's currently failing the autoformatting CI check, and that's preventing us from merging. You'll need to autoformat the two .ql files and push the changes.
|
Fixed |
|
The CI is still complaining: |
|
That's super weird. I autoformatted and pushed changes for both but looks like only one changed looking back at the commit. I autoformatted the second and pushed, hopefully it is okay now. |
I think these are fine to merge in their present form. With the addition of looking at the surrounding context and the functions and classes defined there, I think this might actually be a useful check to have. I don't know of any linters that do this, at any rate. |
RasmusWL
left a comment
There was a problem hiding this comment.
Thanks, this looks fine to me 👍
A minor addition for maintaining standard coding practices when naming functions/classes in python. The two queries simply warn against function names that are capitalized or classes that are not. It is a small/simple addition but important practice that is especially helpful for readability purposes when sharing code with peers.