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

Update Rake tasks #1500

Merged
merged 17 commits into from May 12, 2020
Merged

Update Rake tasks #1500

merged 17 commits into from May 12, 2020

Conversation

pyrmont
Copy link
Contributor

@pyrmont pyrmont commented Apr 14, 2020

A number of the Rake tasks that generate lists of built-in functions for particular languages have bugs. These range from actual errors (as in the case of the Lua task) to deprecation warnings (in the case of tasks using Kernel#open to open URIs).

This PR fixes these bugs and includes freshly generated lists of the built-in functions that have changed.

These changes were originally proposed in #1418.

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Apr 14, 2020
@pyrmont pyrmont self-assigned this Apr 14, 2020
@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 14, 2020

@ashmaroli What do you think? The tasks seemed inconsistent in whether they produced output or not so I also updated them all to be silent (at least if successful). I could see an argument for standardising on a single line for each task stating that it had run successfully (alternatively, possibly something to put in the generate:builtins task).

@ashmaroli
Copy link
Contributor

@pyrmont The changes here look fine. Good to have URI.open instead of Kernel#open even though these are never run by the user.
However, I'm not really sure about the test coverage of these tasks. Are they covered by the unit tests as well or just the visual app..?

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 22, 2020

@ashmaroli None of the Rake tasks have any specific tests. To determine whether they're working correctly, you're correct that it's really just the visual samples that will tell you that. That's consistent with the way the other pre-generated keywords have been handled so I think it's acceptable.

@ashmaroli
Copy link
Contributor

Alrightey then.., LGTM! :)

@pyrmont
Copy link
Contributor Author

pyrmont commented Apr 23, 2020

I decided to go the whole hog and rewrite all the built-in tasks to follow the same format and, as much as possible, use downloaded files rather than static YAML files (the exception is the Matlab YAML file which I can't replace because the Matlab website is purposefully broken when accessing without JavaScript).

Despite the apparent extent of changes, most of the additions and removals are about moving code around into consistent patterns. The big exception is the VimL builtin task. The current VimL lexer uses a binary search (?) to check whether an identifier matches one of the built-in values. This is there from the very earliest version of the VimL lexer and I can't work out why it was done this way. I see no reason that simply checking the existence of the identifier in the auto-generated built-ins won't work so I did that. It seems to be working fine (indeed, the version in this PR works a little better than what we have currently because I included vimFuncName values in the auto-generated built-ins so there are more built-in values matched).

@pyrmont pyrmont merged commit 76430f3 into rouge-ruby:master May 12, 2020
@pyrmont pyrmont deleted the bugfix.rake-tasks-update branch May 12, 2020 17:27
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label May 12, 2020
mattt pushed a commit to NSHipster/rouge that referenced this pull request May 21, 2020
A number of the Rake tasks that generate lists of built-in keywords
for particular languages have bugs. These range from actual errors (as
in the case of the Lua task) to deprecation warnings (in the case of
tasks using `Kernel#open` to open URIs).

This commit fixes these bugs and includes freshly generated lists of
the built-in keywords that have changed.
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

Successfully merging this pull request may close these issues.

None yet

2 participants