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

Rule based editor #151

Merged
merged 22 commits into from
May 10, 2016
Merged

Rule based editor #151

merged 22 commits into from
May 10, 2016

Conversation

jouvin
Copy link
Contributor

@jouvin jouvin commented Apr 23, 2016

This PR imports into CAF the rule-based file editor originally developed as part of ncm-dpmlfc and ncm-xrootd. It intends to address #123.

I'll try to complete the import and constant name cleanup during the week-end. I'd appreciate if this code could go into 16.4 release to help further developments, in particular its use in ncm-dpmlfc and ncm-xrootd. Except if I missed something, it should have no impact on current FileEditor usage as it just adds new methods but don't change anything in the existing code (apart including the module containing the new code).

@jouvin jouvin added this to the 16.4 milestone Apr 23, 2016
@jouvin jouvin changed the title [WIP] Rule based editor Rule based editor Apr 23, 2016
@jouvin
Copy link
Contributor Author

jouvin commented Apr 23, 2016

PR now complete. Constant names for defining line formats have been made independent of their original use case... Comments welcome!

@jouvin
Copy link
Contributor Author

jouvin commented Apr 24, 2016

After thinking a bit more about it, I saw no good reason to have it as a module included by FileEditor rather than a full subclass. In fact it allows to solve the constants duplicated from FileEditor. I implemented this (minor) change in last commit.

@jouvin
Copy link
Contributor Author

jouvin commented Apr 24, 2016

Fixes #123 (I think it covers what was discussed).

# ${build-info}
#
#
# This module implements a rule-based editor that is used to modify the content
Copy link
Member

Choose a reason for hiding this comment

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

use pod style documentation.

Copy link
Member

Choose a reason for hiding this comment

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

ah, if this is the same text as uunder the head1 DESCRIPTION, please remove it here

jouvin added a commit to jouvin/CAF that referenced this pull request Apr 24, 2016
# If the keyword was "negated", remove (comment out) configuration line if present and enabled
if ($comment_line) {
*$self->{LOG}->debug(1, "$function_name: keyword '$keyword' negated, removing configuration line");
$self->_commentConfigLine($keyword, $line_fmt);
Copy link
Member

Choose a reason for hiding this comment

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

handle undef return value (here and below)

@stdweird
Copy link
Member

stdweird commented May 9, 2016

@jouvin i made some more remarks, but nothing major. should be ready to merge

@jouvin
Copy link
Contributor Author

jouvin commented May 9, 2016

I think I addressed everything! I'm about to squash the last commits.

@jouvin
Copy link
Contributor Author

jouvin commented May 9, 2016

Squash done


Defines the format used to represent the keyword/value pair. Several format are supported covering
the most usual ones (SH shell script, Apache, ...). For the exact list, see the definition of
LINE_FORMAT_xxx constants and the associated documnetation below.
Copy link
Member

Choose a reason for hiding this comment

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

documentation typo

@stdweird
Copy link
Member

stdweird commented May 9, 2016

@jouvin some typo's but otherwise good to go (you can safely ignore my ranting about unescape 😉 )

@jouvin
Copy link
Contributor Author

jouvin commented May 9, 2016

No you are right I missed that _a[a-f]will be affected. I'll change the comment, I still think it is harmless in this case normally as you mainly escape path ('/'). I agree that we need to make progress with removing this in panc...

@jouvin
Copy link
Contributor Author

jouvin commented May 9, 2016

@stdweird is the new comment better?

@jouvin
Copy link
Contributor Author

jouvin commented May 9, 2016

Note that the debug info printed should help identify the problem in the unlikely event it occurs...

@stdweird
Copy link
Member

stdweird commented May 9, 2016

LGTM
@jouvin thanks for this!
@ned21 @jrha any other remarks?

@ned21
Copy link
Contributor

ned21 commented May 9, 2016

No comments from me! Great to see this additional functionality. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants