search

epics-base / pvaClientCPP

pvaClientCPP is an EPICS V4 C++ module
http://epics-pvdata.sourceforge.net/index.html
Other
2 stars 8 forks source link

Minor changes #48

Closed mrkraimer closed 6 years ago

mrkraimer commented 6 years ago

Changes for minor problems found while working on exampleJava.

There was one API change: The create method that had arguments for stateChangeRequester and monitorRequester no longer exists.

anjohnson commented 6 years ago

Hi Marty,

Was there a particular reason for removing the channelStateChange methods and constructors?

Apparently pvaPy was using some of those methods you removed, so they weren't unused and this merge has now broken the pvaPy module builds. I wasn't aware of the full details, and Siniša doesn't have time to read and understand the consequences of every pull request in detail, especially given only 4 days notice.

If you want to remove published APIs in the future please mark them EPICS_DEPRECATED for at least a whole release cycle. Only then should you actually remove the implementation from the code. This module has real users now, and the change wasn't a minor one (adding methods can be a minor change, but removing public methods can break downstream code that you don't know about, so should never be thought of as minor).

I think it would be best to revert this merge and redo any other changes that were included in it; there's a button on this github pull-request page which will do the revert for you.

mrkraimer commented 6 years ago

On 01/09/2018 12:51 PM, Andrew Johnson wrote:

Hi Marty,

Was there a particular reason for removing the |channelStateChange| methods and constructors?

Apparently pvaPy was using some of those methods you removed, so they weren't unused and this merge has now broken the pvaPy module builds. I wasn't aware of the full details, and Siniša doesn't have time to read and understand the consequences of every pull request in detail, especially given only 4 days notice.

If you want to remove published APIs in the future please mark them |EPICS_DEPRECATED| for at least a whole release cycle. Only then should you actually remove the implementation from the code. This module has real users now, and the change wasn't a minor one (adding methods can be a minor change, but removing public methods can break downstream code that you don't know about, so should never be thought of as minor).

I think it would be best to revert this merge and redo any other changes that were included in it; there's a button on this github pull-request page which will do the revert for you.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/epics-base/pvaClientCPP/pull/48#issuecomment-356361715, or mute the thread https://github.com/notifications/unsubscribe-auth/AF1Q1jqVFjJio0RVDg2ya7GWw5BB2CYzks5tI6cvgaJpZM4RWdax.

The problem is not the channelStateChange methods, which a client does not see and are never called

I looked and the problem is that I removed a PvaClientMonitor::create method. I should have looked at pvaPy before I removed it. I have reverted the pull request Do I have to merge after the checks complete?