Skip to content

Conversation

@RKrahl
Copy link
Member

@RKrahl RKrahl commented Oct 14, 2021

This amends #89:

  • do not store the JPQL function in the key of Query.conditions but rather set the value to a template string including the JPQL function when present,
  • simplify the code by storing the values of Query.conditions always as a list, even if there is only one single condition,
  • use a regular expression to parse optional JPQL function and attribute.

This addresses the first two bullets in my review of #89 and as a side effect also fixes the error case mentioned in the review. I still might add a few more changes, in particular implementing support for JPQL functions also in order, hence the draft status.

even if there is only one single condition for an attribute.
the keys, but rather set the values to a template string including the
JPQL function when present.
@RKrahl RKrahl added the enhancement New feature or request label Oct 14, 2021
@RKrahl RKrahl added this to the 0.20.0 milestone Oct 14, 2021
@RKrahl RKrahl requested a review from MRichards99 October 14, 2021 10:36
@RKrahl
Copy link
Member Author

RKrahl commented Oct 15, 2021

I now also added JPQL function support for order.

@RKrahl RKrahl marked this pull request as ready for review October 15, 2021 10:41
Copy link
Contributor

@MRichards99 MRichards99 left a comment

Choose a reason for hiding this comment

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

I'm generally happy with this, but could you update the docstring for addConditions as per my comment please?

:param conditions: the conditions to restrict the search
result. This must be a mapping of attribute names to
conditions on that attribute. The latter may either be a
string with a single condition or a list of strings to add
Copy link
Contributor

@MRichards99 MRichards99 Oct 15, 2021

Choose a reason for hiding this comment

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

Please could you change this documentation to remove the section which states that the key could be a string or list of strings? Due to the list comprehension introduced on line 444, the condition key will always be a list, even if it only contains a single string. For example, query.addConditions({"id": "> '5'"}).

I noticed this when running the tests on DataGateway API.

Copy link
Member Author

@RKrahl RKrahl Oct 15, 2021

Choose a reason for hiding this comment

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

I beg to differ. As for any other method, the docstring of addConditions() documents the arguments that the method accepts. It does not document the internal data structures in the attributes of a Query object. These are intentionally left undocumented and thus whetever happens in line 444 is not relevant here. The conditions argument to addConditions() must be a mapping of attribute names to conditions, where the conditions in turn may either be simple strings or list of strings. So this docstring is correct. Your example query.addConditions({"id": "> '5'"}) in fact demonstrates that a simple string value will be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that is something which I didn't initially consider when reading the docstring. I think I was slightly surprised that all keys in self.conditions will always be a list of strings, although I don't think this is a problem. I fixed the tests on DataGateway API and everything behaves as I expect.

Copy link
Member Author

Choose a reason for hiding this comment

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

In any case, we will need to mention in the changelog that the internal data structures in the Query class have changed. This also has an impact on the formal string representation operator __repr__(). I'm going to open another issue on the latter, even if only for the sake of documentaion.

@MRichards99 MRichards99 self-requested a review October 18, 2021 08:21
@RKrahl RKrahl merged commit 38fd671 into sql-functions-conditions Oct 18, 2021
@RKrahl RKrahl deleted the sql-functions-conditions-bis branch October 18, 2021 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants