Note: I assume all change of interest are on in ctrl_events tickets/
DM-1342, so I am ignoring u/srp/ DM-1342.
Thank you for reducing the use of pex_policy. Did you entirely get rid of it? I see it in ctrl_events.cfg and ctrl_events.table, and I see it imported in some code, but I don't see the code actually using it.
It is a complex set of changes. It would help to isolate the different kinds of changes in different commits (or sequences of commits), e.g.:
- getting rid of pex_policy
- adding LocationID and switching to using it
- adding EventAppender
- boilerplate changes that can be pretty much ignored (note that we don't even have to update copyright dates, so some of that wasn't needed)
I don't know how much is practical to do at this point, but if you could improve the coherence of the commits with some squashing then I definitely recommend it. (I also suggest saving a copy of your repo first, in case it doesn't work out.)
I'm very pleased to see that you added some documentation for methods e.g. CommandEvent.cc, LogEvent.cc and StatusEvent.cc. However, more documentation would be really helpful. I'm not necessarily asking you to document existing code on this ticket, but definitely document new code (and existing code would be a bonus). For the rest I request filing a new ticket, or I can file it if you prefer. Examples:
- The .h files generally have very short descriptions of what classes do (and in some cases noted below it's wrong). More information would be a big help, such as what a class is for, an example if relevant, and any design notes that seem relevant.
- For methods with no documentation, please add it.
- For methods with doc headers in the .cc file, please move it to the .h file, so it's centralized. I know at one time we asked it go in the .cc file, but we changed that several years ago.
- It would be great to have an overview of ctrl_events in new file doc/main.dox file. Note that this could replace examples/README (and .dox files can link to code or even include code snippets, which could be handy for those examples).
LocationID is a very nice improvement. I do have some minor suggestions:
- What does _localCounter do? I see you increment it when the default constructor is called, but I don't see any code that reads it or uses it in any way. If it's not being used then I suggest you eliminate it. If you want to keep it please document it.
- Do you need an explicit copy constructor? I think C++ will provide you a default one that works fine (but please verify that). If you do keep it, I suggest putting it after the normal constructors and documenting it as a copy constructor.
- Please rename to LocationId (lowercase d) to meet our naming standard (alas).
CommandEvent.h: one trivial change: remove "// protected:". Congratulations on getting rid of that section.
- Why did you add the use of boost propert_tree?
- The doc header for EventAppender says "Representation of an LSST Event", which surely isn't right? It is the same header as for Event.
- Please also document the member functions and possibly the instance variables.
- I am alarmed to see many methods returning bare pointers instead of shared_ptr. Doesn't that make memory management a headache? createOriginatorId() is new and definitely should be changed. I suggest also changing the others or filing a ticket.
- This class uses bare pointers for instance variables. Again, please use shared_ptr or unique_ptr. It's a lot safer, especially when handling exceptions during construction or destruction. I realize it's existing code, so it may be worth a new ticket.
- Its doc header says "Coordinate publishing and receiving events". Surely not?
- getHost doc says "\return tThe EventSystem object". Typo? Also, please consider putting the doc in the header. I'm pretty sure that's our standard now (originally it was the other way, but was changed years ago).
- This sort of thing is easier and faster using set (and indeed I'd cast values to a set immediately in checkValidity, since you don't care about order):
for x in values:
self.assertTrue(x in names)
e.g. self.assertTrue(set(values) < set(names)), or if they're supposed to match, use ==
tests/ComplexData.py: this is a bit clumsy:
- should never reach here
self.assertTrue(1 == 0)
except ex.Exception as e:
print "exception thrown, as expected"
how about this instead:
self.assertRaises(ex.Exeption, trans.publishEvent, event)
or at least this if you prefer not to use assertRaises for some reason:
self.fail("exception not raised")
Unused imports in unit tests, as found by pyflakes:
- CommandEvent.py: policy
- ComplexData.py: time
- ESCombinedTopic.py: time
- ESTransmitReceive.py: time
- Event.py: getHostname; also, this is unused: status = "my special status"
- EventAppenderTest: sys, lsst.ctrl.events
- EventFilters.py: gethostname
- EventLogging.py: PropertySet
- EventSystem_LogRecord.py: lsst.pex.exceptions
- EventTransmitter.py: policy
- LocationId.py: struct, base, lsst.pex.exceptions, logging, policy
- StatusEvent.py: time
- SendEvent.py: gethostname
New C++ code (it may not be worth changing existing code):
- I see pex/policy.h included in many places, but I'm not sure I see it used anywhere.
- I suggest eliminating _constructor and having the other constructors call the main constructor. Before C++11.
- Please put const after the type, e.g. "std::string const &hostname" instead of "const std::string &hostname"; this is a coding standard.
- The code would be a bit easier to read if you closed namespaces with all closing braces on one line (e.g. }}}), preferably after a blank line.
New Python code:
- Please run a linter such as "pyflakes" on python code. This can catch serious errors as well as more mundane things such as unused imports and lines that don't do anything. Many editors (including the amazing Sublime Text) can be configured to do this automatically.
- I suggest using "foo is None" instead of "foo == None" because it is standard for Python and is the only thing that works for numpy arrays. I'm not sure any new code has this.