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

Add = as Operator in Puppet lexer #980

Merged
merged 2 commits into from
Sep 14, 2018

Conversation

alexharv074
Copy link
Contributor

The = sign had been missed apparently as a valid Puppet operator.
Fixes #979.

Copy link
Collaborator

@dblessing dblessing left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @alexharv074. Couple of comments.

spec/lexers/puppet_spec.rb Outdated Show resolved Hide resolved
lib/rouge/lexers/puppet.rb Outdated Show resolved Hide resolved
@dblessing dblessing added the author-action The PR has been reviewed but action by the author is needed label Sep 4, 2018
The = sign had been missed apparently as a valid Puppet operator. Also
adds tests. Fixes rouge-ruby#979.
@alexharv074
Copy link
Contributor Author

@dblessing , I refactored to avoid the confusing syntax as you suggested. Meanwhile, I left the Rspec tests in. I think they assist any future maintainers to get their heads around why a 2013-era syntax highlighter actually works on the latest version of Puppet?

@dblessing
Copy link
Collaborator

@gfx @jneen Do you have an opinion on the tests? We have some similar tests in the code-base but I recall @jneen telling me once that they aren't needed like this.

@dblessing
Copy link
Collaborator

I think they assist any future maintainers to get their heads around why a 2013-era syntax highlighter actually works on the latest version of Puppet?

FWIW Rouge is constantly updated and lexers are added, updated, etc. as needed. When Rouge was created has no bearing one whether it does or doesn't support the latest version of some language.

@jneen
Copy link
Member

jneen commented Sep 4, 2018

I'd generally prefer adding the example to spec/samples/puppet, since I've found the usual-style tests to be quite brittle when we have to accomodate other language changes.

Based on discussion on the PR, it is agreed to add sample code in
spec/visual/samples/puppet and not have the Rspec tests that I added.
@alexharv074
Copy link
Contributor Author

alexharv074 commented Sep 5, 2018

@dblessing @jneen for what it's worth, I found it much easier to analyse the behaviour of the lexers using the Rspec framework than I did using the debug output from rougify so an understanding of these tests should help maintainers develop their code. But with that said, I removed the tests and added sample code in spec/visual/samples/puppet as requested. Those tests can be restored by checking out e1ad61a.

@alexharv074
Copy link
Contributor Author

@dblessing not sure if you're waiting on me here, but I can't find any way from my end to move this Issue out of "Changes requested" state. Let me know if there's anything else I am supposed to to?

@alexharv074
Copy link
Contributor Author

Any chance this could be merged now @dblessing ?

@dblessing dblessing merged commit 7c598c0 into rouge-ruby:master Sep 14, 2018
@alexharv074
Copy link
Contributor Author

@dblessing Looks like this was merged over 6 months ago but still my GitHub/Jekyll blog doesn't have the fix. Any idea why that might be?

@dblessing
Copy link
Collaborator

dblessing commented Mar 25, 2019 via email

@alexharv074
Copy link
Contributor Author

Yeah, I just specify latest jekyll on my side and the rest happens magically on the GitHub side. Ok, thanks for merging and releasing anyway! I'll investigate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author-action The PR has been reviewed but action by the author is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants