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.

Sign up for GitHub

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

Jump to bottom

Cleanup and unittest Object, Log, Reporter and ReporterMany #103

Merged
merged 11 commits into from
Dec 5, 2015
Merged

Cleanup and unittest Object, Log, Reporter and ReporterMany #103

merged 11 commits into from
Dec 5, 2015

Conversation

stdweird
Copy link
Member

@stdweird stdweird commented Sep 3, 2015

This is preparation for #100 and #93
Fixes #102 (not via removal, but through proper subclass of Reporter)

minor backwards incompatibility:

  • subclassed ReporterMany has different/fixed/correct setup_reporter behaviour
    • be able to turn off quiet and verbose
    • verbose triggered by debug
    • init with global _REP_SETUP happens on usage via _rep_setup method, not on setup_reporter (i.e. it will report things without setup_reporter being called if global Reporter::_REP_SETUP is set to do so).

@stdweird stdweird added this to the 15.10 milestone Sep 3, 2015
@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/CAF-pr-builder/232/

@stdweird stdweird changed the title Cleanup and unittest Object, Log and Reporter Cleanup and unittest Object, Log, Reporter and ReporterMany Sep 4, 2015
@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/CAF-pr-builder/233/

@hpcugentbot
Copy link

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/CAF-pr-builder/234/

@stdweird
Copy link
Member Author

@ned21 any toughts on this? (esp the modified log format)

ned21
ned21 reviewed Oct 4, 2015
View reviewed changes
src/main/perl/Log.pm

closes the log file.
closes the log file, returns SUCCESS on succes, undef otherwise
Copy link
Contributor

Choose a reason for hiding this comment

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

typo: success

@ned21
Copy link
Contributor

ned21 commented Oct 4, 2015

@stdweird sorry it took my so long to get round to reviewing this it seems the new log format was reverted anyway?

@stdweird stdweird force-pushed the split_loglevels branch 2 times, most recently from 79838ca to d1d67ea Compare October 5, 2015 06:00
@stdweird
Copy link
Member Author

stdweird commented Oct 5, 2015

@ned21 thanks for reviewing
wrt the log format, i opened #115. i'm fine with keeping the old format, just wanted to point out this (small) inconsistency

@stdweird stdweird force-pushed the split_loglevels branch 2 times, most recently from 41e1f6f to 64ab71f Compare October 5, 2015 06:19
@stdweird stdweird mentioned this pull request Oct 10, 2015
Merged
@stdweird
Reporter: cleanup of code (no modified functionality/behaviour) and a…
c59602d 
…dded unittests
@stdweird
Move test modules to new modules subdir
6bdd501
@stdweird
Object: cleanup of code (no modified functionality/behaviour) and add…
a4a1184 
…ed unittests
@stdweird
Log: cleanup of code (no modified functionality/behaviour) and added …
bbd4e8d 
…unittests
@stdweird
Reporter: make changes to be able to make ReporterMany a subclass of …
664e2d7 
…Reporter
@stdweird
ReporterMany: make it a proper subclass of Reporter
c37e30c
@stdweird
Add unittests for ReporterMany to the reporter unittest (makes the di…
6077bfe 
…fference between Reporter and ReporterMany very clear).
@stdweird
Log: use the exported Reporter SYSLOG readonly
63399cf
@stdweird
Log uses Readonly during DESTROY, which might be already cleaned up t…
9be1abf 
…hemself during global cleanup.
@stdweird
Reporter: revert to old log prefix format
1ba3bd5
@stdweird
FileWriter: reported diff cannot be undef with 'use warnings'
0f36555
@stdweird
Copy link
Member Author

@ned21 @jrha anything left to do?

@jrha
Copy link
Member

jrha commented Nov 30, 2015

LGTM

@stdweird
Copy link
Member Author

stdweird commented Dec 4, 2015

@ned21 ping

ned21
ned21 reviewed Dec 4, 2015
View reviewed changes
src/main/perl/FileWriter.pm
@@ -173,8 +172,8 @@ sub close
keeps_state => 1);
$cmd->execute();
*$self->{LOG}->verbose ("Changes to ", *$self->{filename}, ":");
$diff = "" if (! defined($diff));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this cause report to log an empty line or is it suppressed by the logging stack? Is an empty line the right way of indicating an empty diff or should we print something standardised such as "(no changes)" ?

Copy link
Member Author

Choose a reason for hiding this comment

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

this change is to get rid of some 'undefined' warnings (and i think only in the unittests). there's also #119 to handle empty diffs

@ned21
Copy link
Contributor

ned21 commented Dec 4, 2015

Aside from the one question I think this is good to merge.

ned21 added a commit that referenced this pull request Dec 5, 2015
@ned21
Merge pull request #103 from stdweird/split_loglevels
d364747 
Cleanup and unittest Object, Log, Reporter and ReporterMany
@ned21 ned21 merged commit d364747 into quattor:master Dec 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
15.12
Development

Successfully merging this pull request may close these issues.

remove ReporterMany (or explain what is supposed to add to Reporter)
4 participants
@stdweird @hpcugentbot @ned21 @jrha