Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

object_camel_case_linter is over-eager #108

Closed
klmr opened this issue Jul 26, 2015 · 5 comments
Closed

object_camel_case_linter is over-eager #108

klmr opened this issue Jul 26, 2015 · 5 comments

Comments

@klmr
Copy link
Contributor

klmr commented Jul 26, 2015

I get multiple errors “Variable and function names should be all lowercase” for using predefined names such as .GlobalEnv or getRegisteredNamespace (the latter is an internal name, called via .Internal(getRegisteredNamespace(…))) or context$cppSourceFilename.

All of these cases are names from elsewhere that my package is using, not defining. In other words, I have no control over them. Currently, the linter only excludes names found in attached packages but as the above shows, this leads to many false positives. Ideally, the linter should only complain about names that are defined in the package (i.e. that are on the left side of an assignment operator or, optionally, the first argument of assign, or are inside a function argument definition).

There’s one complication in the proposal: it needs to special-case c and list calls to ensure that the following code raises the warning:

x = list(fooBar = 1)

The same is probably true for object_snake_case_linter.

@jimhester
Copy link
Member

.GlobalEnv is defined in the base functions, so should definitely be excluded in recent versions of lintr (see jimhester/lintr/R/objects_linter.R#L120-L161)

As you state getRegisteredNamespace is a internal name, so there is no way to retrieve them without parsing the definitions of all the base functions. Also I thought that using internal functions in user package code was frowned upon by CRAN, so I am not clear when you would encounter this situation.

Determining variables defined by a package is actually non-trivial to do (and in some cases impossible without evaluating the code), so I am unsure if that is the best approach here. Also once a user fixes the assignment (e.g. changing fooBar to foo_bar), should the other instances of the variable fooBar in the package continue to be linted as camelCase, or do we then assume they come from an external source? I would argue they should continue to be linted as before, but it is not clear to me how that would be done with your proposal.

I do agree however that there are still more false positive lints due to external definitions than I would like, perhaps more can be done to import the exported namespaces of packages in Imports, Depends, Suggests to reduce this issue.

@klmr
Copy link
Contributor Author

klmr commented Jul 27, 2015

Determining variables defined by a package is actually non-trivial to do (and in some cases impossible without evaluating the code), so I am unsure if that is the best approach here.

There’s no perfect way. However, as we’ve seen all alternatives lead to errors, and I think that implementing the strategy I suggested would significantly lower the false positive rate, while barely increasing the false positive rate. However this is just a hunch, I have no evidence either way, and my suggestion does have some caveats.

Also I thought that using internal functions in user package code was frowned upon by CRAN

My package is not on CRAN (for this and similar reasons). But I need to access R internals because R doesn’t otherwise expose the proper API for my purposes (my package modules is recreating parts of R’s package mechanism).

@klmr
Copy link
Contributor Author

klmr commented Jul 28, 2015

Incidentally, I don’t know why .GlobalEnv wasn’t white-listed: I had installed lintr from Github on the day of filing this issue. Nevertheless, I’ve now re-installed it and it no longer flags .GlobalEnv. This means that the issue is much less relevant now.

@jankatins
Copy link

Things like

\kernel.r:291:25: style: Variable and function names should be all lowercase.
    connection_info <<- fromJSON(connection_file)
                        ^~~~~~~~

are definitely false positive...

IMO it would also be enough to check that an object is assigned something (e.g. <- or = after the offending thing or -> before the object) -> fromJSON( isn't an assignment, so should be skipped. Similar for if(runLast).

@fangly
Copy link
Contributor

fangly commented Mar 13, 2017

This is fixed in #214 . Please, do open a new issue if you find some names that get linted when they should not (or inversely).

@fangly fangly closed this as completed Mar 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants