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

RuleBasedEditor suggested improvements #159

Open
jouvin opened this issue May 9, 2016 · 3 comments
Open

RuleBasedEditor suggested improvements #159

jouvin opened this issue May 9, 2016 · 3 comments
Assignees

Comments

@jouvin
Copy link
Contributor

jouvin commented May 9, 2016

During #151 review of the initial RuleBasedEditor code, some areas for future improvements have been identified:

  • Use a dispatch table to replace the if...elsif.. blocks related to line formats and value formats in several methods to make the maintenance easier and the code easier to extend. This could possibly allow to support formats not defined in the base RuleBasedEditor.
  • _parse_rules() contains two if ( $negate ) ... blocks that implement an XOR. Would be better replaced by PErl xor but attempt to do it during Rule based editor #151 failed...
  • In _apply_rules: decide if a failed rule should be signaled with an error or warning. Note that this happens mainly because of internal errors (RuleBasedEditor bugs or incorrect rules, e.g. incorrect format paramaters). And that if it should be a warning, this probably means that internal errors in other methods should also be warnings...
  • Add a specific unit test for _parse_rules() method rather than rely on the _apply_rules() test (rbe_rule_parser.t).
@jouvin jouvin added this to the 16.6 milestone May 9, 2016
@stdweird
Copy link
Member

stdweird commented May 9, 2016

  • consistent return values: undef on failure, SUCCESS on success (see eg _commentConfigLine) adn act upon them in _apply_rules

@stdweird
Copy link
Member

  • export the constants also as readonly, so the can used as variable in the string rather than concatting, eg.
    use "DiskFlags:dpns->DiskFlags:dav;$LINE_FORMAT_KW_VAL;$LINE_VALUE_ARRAY", rather than current "DiskFlags:dpns->DiskFlags:dav;".LINE_FORMAT_KW_VAL.";".LINE_VALUE_ARRAY,
  • refactor the unittest code by introducing some function instead of copy/pasting the tests
  • unittests should be more abstract and try to test all combinations

@stdweird
Copy link
Member

stdweird commented Aug 4, 2016

@jouvin moving this milestone, afaik, you are on holiday for the remainder of 16.8

@stdweird stdweird modified the milestones: 16.10, 16.8 Aug 4, 2016
@jrha jrha removed this from the 16.10 milestone Oct 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants