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.

Sign up for GitHub

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

Jump to bottom

Use lint text if supported #284

Merged
merged 15 commits into from
Apr 29, 2021
Merged

Use lint text if supported #284

merged 15 commits into from
Apr 29, 2021

Conversation

renkun-ken
Copy link
Member

@renkun-ken renkun-ken commented Jun 25, 2020 •
edited
Loading

Close #80
Close #235
Close #238
Close #282

r-lib/lintr#503 adds a text= argument to lintr::lint to make it easier for code editors to lint a text as if it is from a filename so that lint config is kept effective.

@randy3k
Copy link
Member

randy3k commented Jun 26, 2020

LGTM except you might need to redocument. Actually do we really want to document all the internal functions? If they are not needed, perhaps it is easier to remove those documentations.

@renkun-ken
Copy link
Member Author

Since r-lib/lintr#503 is merged, we could wait for the latest lintr (probably 3.0.0) to be released, then we could import lintr (>= 3.0.0) and this PR could be merged.

@renkun-ken
Copy link
Member Author

renkun-ken commented Apr 4, 2021 •
edited
Loading

@randy3k Since we have support for both old lintr (current release) and latest lintr with lint(text=) by checking if the text argument exists, I guess we could merge this without having to rely on the upcoming lintr >= 3.0.0?

randy3k
randy3k approved these changes Apr 6, 2021
View reviewed changes
Copy link
Member

@randy3k randy3k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit hesitated to merge it. With that say, I guess it is an acceptable solution for now, just need to put a note here to remind ourselves in the future.

R/diagnostics.R Outdated Show resolved Hide resolved
@renkun-ken
Copy link
Member Author

renkun-ken commented Apr 7, 2021 •
edited
Loading

The tests only work with latest lintr rather than the current CRAN release. I guess we should postpone this PR until lintr 3.0.0 is released and then we could directly import lintr 3.0.0 and do not have to detect text= like this and those test cases should work too.

Or after this languageserver release, we could put back Remotes: jimhester/lintr for its latest development version in the languageserver dev version.

@renkun-ken
Copy link
Member Author

I guess we could merge this PR into master now. The development version of lintr is required until lintr 3.0.0 is released to CRAN.

randy3k
randy3k reviewed Apr 29, 2021
View reviewed changes
R/diagnostics.R Outdated
# In the upcoming lintr 3.0.0, lint(text=) is supported so that
# we could supply both filename and text and the text will be
# regarded as the content of the file.
lint_text_support <- "text" %in% names(formals(lintr::lint))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this for the version of lintr specified in DESCRIPTION?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now we could safely assume lintr(text=). I'll remove those checking.

@randy3k
Copy link
Member

randy3k commented Apr 29, 2021

LGTM

@renkun-ken renkun-ken merged commit 4280b4c into REditorSupport:master Apr 29, 2021
@randy3k
Copy link
Member

randy3k commented Jun 6, 2021

I just realized that there is no roadmap for the release of new version of lintr. Perhaps we should include your original trick of checking the text parameter for now? Otherwise we won't know how much time they need to make a new release.

@randy3k randy3k mentioned this pull request Jun 6, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@randy3k randy3k randy3k approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@renkun-ken @randy3k