You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/03 09:05:37 UTC
[GitHub] [incubator-superset] villebro opened a new pull request #9460:
pylint: accept 2 character names by default
villebro opened a new pull request #9460: pylint: accept 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [x] Build / Development Environment
- [ ] Documentation
### SUMMARY
We were already accepting `[a-z_][a-z0-9_]{1,30}$` for variable names, but not argument names, as `pylint` defaults to `[a-z_][a-z0-9_]{2,30}$` for names in general. As we're heavy users of Pandas aka `df`, it makes sense to accept these by default.
### REVIEWERS
@john-bodley
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro merged pull request #9460: pylint:
accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#9460: pylint: accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#discussion_r404809197
##########
File path: .pylintrc
##########
@@ -115,10 +115,10 @@ evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / stateme
[BASIC]
# Good variable names which should always be accepted, separated by a comma
-good-names=i,j,k,ex,Run,_,d,e,v,o,l,x,ts,f
+good-names=_,d,df,e,ex,f,i,id,j,k,l,o,pk,Run,ts,v,x
Review comment:
nice!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9460: pylint: accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#discussion_r404873844
##########
File path: .pylintrc
##########
@@ -115,10 +115,10 @@ evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / stateme
[BASIC]
# Good variable names which should always be accepted, separated by a comma
-good-names=i,j,k,ex,Run,_,d,e,v,o,l,x,ts,f
+good-names=_,d,df,e,ex,f,i,id,j,k,l,o,pk,Run,ts,v,x
Review comment:
I think `e` should be a bad name. We should favor `ex` for exceptions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9460: pylint:
accept 2 character names by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9460: pylint: accept 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-608615015
@villebro `ex` is already in the list of [good_names](https://github.com/apache/incubator-superset/blob/master/.pylintrc#L118) which is a `pylint` default. I think in the future we should rename `e` to `ex`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9460: pylint:
accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-611053157
@john-bodley if you care to take another look, this turned into quite the refactor PR 😁
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9460: pylint:
accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-610241180
@john-bodley @etr2460 this is ready for another round of review.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9460: pylint:
accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-610392669
@dpgaspar are you referring to `pk`? That's added here, but it seems a few new ones have popped up since last rebase, let me remove those. Are there any other ones we should address here, too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] villebro commented on issue #9460: pylint:
accept 2 character names by default
Posted by GitBox <gi...@apache.org>.
villebro commented on issue #9460: pylint: accept 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-608564469
I'm fine with that approach, too. Personally I feel the three char minimum imposed by `pylint` is slightly overkill, but agree that excessive use of very short names is bad practice. If we go the whitelist route, I propose whitelisting `id` (class attribute), `df`, `fd` (form_data) and perhaps `ex` for exceptions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on a change in pull
request #9460: pylint: accept specific 2 character names by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on a change in pull request #9460: pylint: accept specific 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#discussion_r404873107
##########
File path: .pylintrc
##########
@@ -115,10 +115,10 @@ evaluation=10.0 - ((float(5 * error + warning + refactor + convention) / stateme
[BASIC]
# Good variable names which should always be accepted, separated by a comma
-good-names=i,j,k,ex,Run,_,d,e,v,o,l,x,ts,f
+good-names=_,d,df,e,ex,f,i,id,j,k,l,o,pk,Run,ts,v,x
# Bad variable names which should always be refused, separated by a comma
-bad-names=foo,bar,baz,toto,tutu,tata,d,fd
+bad-names=d,fd,foo,bar,baz,toto,tutu,tata
Review comment:
How is `d` both a good and bad name?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on issue #9460: pylint:
accept 2 character names by default
Posted by GitBox <gi...@apache.org>.
etr2460 commented on issue #9460: pylint: accept 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-608557575
Definitely agree with John's comment here, I'd rather we whitelist the specific 2 char variable names we think are acceptable.
Honestly, I wonder if we should change the variable name rules to match the other ones too
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] john-bodley commented on issue #9460: pylint:
accept 2 character names by default
Posted by GitBox <gi...@apache.org>.
john-bodley commented on issue #9460: pylint: accept 2 character names by default
URL: https://github.com/apache/incubator-superset/pull/9460#issuecomment-608550275
@villebro I think the `pylint` defaults (including three character variable names) are normally defined for a good reason. Why not just whitelist `id`, `df`, etc?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org