search

bblfsh / sdk

Babelfish driver SDK
GNU General Public License v3.0
23 stars 27 forks source link

More DSL operations to check objects #345

Closed dennwc closed 5 years ago

dennwc commented 5 years ago

This is a combination of a few smaller changes required for C# driver.

First, it contains a change that breaks backward compatibility of one rarely used op for the sake of better naming. Previously Any(sel) operator was used to check if "any node in the array matches sel". PR renames this operator to AnyElem and defines a new function Any() that acts like "any node". This will allow us to replace a common pattern of AnyNode(nil) with Any(), which should be more readable.

The change won't break any drivers until they decide to update SDK version. I propose to just bump a minor version since the operation is very rare and the fix will only require to change the function name.

Next, PR defines a HasFields helper that can be used to check for presence or absence of specific object fields. Instead of Not(Has{k1: Any()}) it's now possible to write HasFields{k1: false}.

To make the new operator useful, PR also defines a CheckObj helper that will properly propagate object-related information to the parent.

It also adds or clarifies documentation for related types.

The change may improve the performance of AnnotateIfNoRoles since it will be now able to propagate constraints to the root of the transform tree.

juanjux commented 5 years ago

Nothing like writing a new driver to discover possible usability improvements!

Loving these changes but you know what I'm going to say... need some tests :)

dennwc commented 5 years ago

@creachadair In general, I agree that those breaking changes should not be accepted lightly. But in this particular case, it's really internal detail of an SDK (no 3rd-party drivers) and only Java driver uses Any (I've checked it). So I think it's safe to apply the change until it's too late and update that single driver (it's <30 mins of work).

creachadair commented 5 years ago

@creachadair In general, I agree that those breaking changes should not be accepted lightly. But in this particular case, it's really internal detail of an SDK (no 3rd-party drivers) and only Java driver uses Any (I've checked it). So I think it's safe to apply the change until it's too late and update that single driver (it's <30 mins of work).

That's reasonable. I would just like to minimize the amount of state someone (and in this case I'm selfishly thinking of myself) has to keep in mind when making changes to other parts of the system, so that it will be easier to work in parallel.

juanjux commented 5 years ago

I'm ok with merging this if we inmediately (in the sense of not waiting for the next SDK update to avoid surprises and maybe find possible bugs now) fix all the drivers needing or benefitting from it so our code remains "idiomatic". AFAIK, this would be the Java driver (incompatible changes) and the C++ driver (replacing AnyNode(nil) for Any()).

dennwc commented 5 years ago

Sounds good. I can update both.

dennwc commented 5 years ago

Added a test case. PTAL.

dennwc commented 5 years ago

As a bonus, I've added the test for CaseObj and documented it. Codecov is finally happy :)