Fixing ruby Symbol regex handler.#288
Conversation
mvsm
force-pushed
the
master
branch
7 times, most recently
from
f806dc9 to
143f20a
Compare
kenyon
left a comment
kenyon
left a comment
There was a problem hiding this comment.
What is causing these formatting changes? It's a bit difficult to see the actual changes when formatting changes are mixed in.
|
I fixed to pass the CI configuration. It was complaining and not passing all checks. I can undo them if required. |
kenyon
left a comment
kenyon
left a comment
There was a problem hiding this comment.
The linting changes probably don't need to be undone, but it's nice to have them in a separate commit for ease of review. I'm not too familiar with this module, so we'll let someone else review, but otherwise this looks OK to me.
| $missing_backends = difference($backends, keys($backend_data)) | ||
| if count($missing_backends) > 0 { | ||
| fail("The supplied backends: ${missing_backends} are missing from the backend_options hash:\n ${backend_options}\n | ||
| fail("The supplied backends: ${missing_backends} are missing from the backend_options hash:\n ${backend_options}\n |
There was a problem hiding this comment.
This seems like worse formatting, the indentation doesn't line up.
There was a problem hiding this comment.
I agree. I just followed the CI (history here https://travis-ci.org/github/voxpupuli/puppet-hiera/pull_requests) until it was all green.
There was a problem hiding this comment.
I can undo the changes and separate into two different commits, I really don't mind. I sent as one commit because some repos only accept pull requests with one commit. Regardless, it is your call, I just want to help! =)
ghoneycutt
left a comment
ghoneycutt
left a comment
There was a problem hiding this comment.
Could you please add a test that currently fails and will pass after this change to demonstrate the issue with the regexp?
Also regexp's are hard to read and understand what the author's intentions are. Could you please add a comment to the erb that explains what this is attempting to match.
<%-# This is a sample comment! %>
|
I can add the test and the comments. I will open an issue with the test case, is that OK? |
| }, | ||
| 'redis' => { | ||
| 'deserialize' => '!ruby/sym json' | ||
| } |
There was a problem hiding this comment.
is adding this backend the reason for this pr?
it's not clear from the title and I'm so confused about half of these changes
There was a problem hiding this comment.
The reason behind the changes is because the regex to parse symbols is not working properly. This backend is a test case (that also is mentioned in the readme). When I was actually trying to use the mechanism to convert the value into a symbol, the output of this '!ruby/sym json' was becoming this ':undef'. The changes were only on the template file, everything else was because the CI was requesting formating to pass, I can revert them, but you will need to fix the CI or accept without a green ack.
mvsm
changed the title
|
I formally created an issue (#289), I just can't link it. |
igalic
left a comment
igalic
left a comment
There was a problem hiding this comment.
there's a lot of whitespace changes to please the linter, which i don't agree with
what i do agree with is the code changes
|
Any news? |
|
Dear @mvsm, thanks for the PR! This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
4 participants
Pull Request (PR) description
The goal of this pull request is to fix the regex in place to handle ruby symbols. It was not handling it properly, always outputting ":undef".
This Pull Request (PR) fixes the following issues
Fixes #289.
Thanks!