On Tue, Apr 5, 2022 at 12:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Apr 5, 2022 at 6:21 AM Peter Smith <smithpb2...@gmail.com> wrote: > > > > Here are my comments for the latest patch v6-0001. > > > > (I will post my v6-0002 review comments separately) > > > > PATCH v6-0001 comments > > ====================== > > > > 1.1 General - Option name > > > > I still feel like the option name is not ideal. Unfortunately, this is > > important because any name change would impact lots of these patch > > files and docs, struct members etc. > > > > It was originally called "local_only", but I thought that as a > > SUBSCRIPTION option that was confusing because "local" means local to > > what? Really it is local to the publisher, not local to the > > subscriber, so that name seemed misleading. > > > > Then I suggested "publish_local_only". Although that resolved the > > ambiguity problem, other people thought it seemed odd to have the > > "publish" prefix for a subscription-side option. > > > > So now it is changed again to "subscribe_local_only" -- It's getting > > better but still, it is implied that the "local" means local to the > > publisher except there is nothing in the option name really to convey > > that meaning. IMO we here all understand the meaning of this option > > mostly by familiarity with this discussion thread, but I think a user > > coming to this for the first time will still be confused by the name. > > > > Below are some other name ideas. Maybe they are not improvements, but > > it might help other people to come up with something better. > > > > subscribe_publocal_only = true/false > > origin = pub_only/any > > adjacent_data_only = true/false > > direct_subscriptions_only = true/false > > ... > > > > I think local_only with a description should be okay considering other > current options. Some of the options like slot_name, binary, etc. are > publisher-specific which becomes clear only after reading the > description. I am not sure adding pub*/sub* to it is much helpful. So, > +1 to a simple boolean like 'local_only', 'data_local', or something > like that.
+1 to use a simple boolean option. Thanks for the examples of the other options which are also really publisher-specific. Seen in that light, and with a clear description, probably the original "local_only" was fine after all. ------ Kind Regards, Peter Smith. Fujitsu Australia
