-
Notifications
You must be signed in to change notification settings - Fork 38
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
Fixes #5550: correct classes conditions when using quotes #123
Fixes #5550: correct classes conditions when using quotes #123
Conversation
@jooooooon could you review this please ? |
When fixing a bug in ncf, please always adjust the tests so that this bug can never come back. Aside from that, the fix looks OK, but only applies to quotes. Is that deliberate? What about other cases that use the escape character? I think that CFEngine allows to escape any character, am I wrong? |
it seems that only ' and " are escapable (according to my tests) however, I have no idea on how to make these unit tests :/ |
ha, apparently, this doesn't report correctly, as CFEngine convert the ' to ' in the sent message... |
ok, so canonify does canonify correctly \c to _c, and 'c to _c , but somehow our method to generate the .res seems to drop the \ in \c |
3803d26
to
d295afd
Compare
I've updated the PR and FAIL: test_parse_technique_generic_method_calls (main.TestNcf) |
d295afd
to
38429a5
Compare
PR updated, with tests |
@@ -271,7 +273,7 @@ def generate_rudder_reporting(technique): | |||
method_name = method_call['method_name'] | |||
generic_method = generic_methods[method_name] | |||
|
|||
key_value = method_call["args"][generic_method["class_parameter_id"]-1] | |||
key_value = method_call["args"][generic_method["class_parameter_id"]-1].replace("\\'", "\'").replace('\\"', '\"') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"'" is equivalent to "'" in python
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this one, you added 2 more backslash before and here you remove only one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess this is not relevant anymore; we changed a lot of things around that (notably, we don't use bash anymore to generate the reporting file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not relevant, should we remove some or all of this PR ?
actually, this is still relevant :( |
replaced by #223 |
No description provided.