Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the search result in the case of multiple fields from list to tuple #82

Merged
merged 5 commits into from Apr 13, 2021

Conversation

RKrahl
Copy link
Member

@RKrahl RKrahl commented Apr 13, 2021

While working on #81, I noticed another issue in the results of a searching for multiple fields: the Client.search() method returns a list of the results. There are situation where it may be convenient to turn that list into a set and in general this works. Note that Entity objects are hashable even though they are mutable and that has been made particularly with the intention to allow them to be added as an item to a set.

Now:

>>> query = Query(client, "Investigation")
>>> res = client.search(query)
>>> set(res)
{(investigation){
   createId = "simple/root"
   createTime = 2021-04-13 08:47:18+02:00
   id = 31
   modId = "simple/root"
   modTime = 2021-04-13 08:47:18+02:00
   doi = "00.0815/inv-00122"
   name = "08100122-EF"
   startDate = 2008-03-13 11:39:42+01:00
   title = "Durol single crystal"
   visitId = "1.1-P"
 }, (investigation){
   createId = "simple/root"
   createTime = 2021-04-13 08:47:19+02:00
   id = 33
   modId = "simple/root"
   modTime = 2021-04-13 08:47:19+02:00
   doi = "00.0815/inv-00409"
   endDate = 2012-08-06 03:10:08+02:00
   name = "12100409-ST"
   startDate = 2012-07-26 17:44:24+02:00
   title = "NiO SC OF1 JUH HHL"
   visitId = "1.1-P"
 }, (investigation){
   createId = "simple/root"
   createTime = 2021-04-13 08:47:19+02:00
   id = 32
   modId = "simple/root"
   modTime = 2021-04-13 08:47:19+02:00
   doi = "00.0815/inv-00601"
   endDate = 2010-10-12 17:00:00+02:00
   name = "10100601-ST"
   startDate = 2010-09-30 12:27:24+02:00
   title = "Ni-Mn-Ga flat cone"
   visitId = "1.1-N"
 }}
>>> query = Query(client, "Investigation", attributes="name")
>>> res = client.search(query)
>>> set(res)
{12100409-ST, 08100122-EF, 10100601-ST}
>>> query = Query(client, "Dataset", attributes="investigation.name")
>>> res = client.search(query)
>>> res
[08100122-EF, 08100122-EF, 10100601-ST, 10100601-ST, 10100601-ST, 12100409-ST, 12100409-ST, 12100409-ST]
>>> set(res)
{12100409-ST, 08100122-EF, 10100601-ST}
>>> query = Query(client, "Dataset", attributes=("name", "investigation.name")) 
>>> res = client.search(query)
>>> res
[[e201215, 08100122-EF], [e201216, 08100122-EF], [e208339, 10100601-ST], [e208341, 10100601-ST], [e208342, 10100601-ST], [e208945, 12100409-ST], [e208946, 12100409-ST], [e208947, 12100409-ST]]
>>> set(res)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unhashable type: 'list'

In the case of searching for multiple fields, turning the result into a set fails, because it is a list of lists with attribute values. The present PR changes this to a list of tuples instead.

@RKrahl RKrahl added the enhancement New feature or request label Apr 13, 2021
@RKrahl RKrahl added this to the 0.18.1 milestone Apr 13, 2021
@RKrahl
Copy link
Member Author

RKrahl commented Apr 13, 2021

Note that strictly speaking, this is an incompatible change. I still list it as minor change in the change log, because first of all, I assume that most users will not even notice the difference, because tuples mostly behave like lists. Furthermore it changes the behavior for a case that has been implemented very recently in 0.18.0, so that there probably does not yet exist any code out there that may be affected.

@MRichards99, I assume you are the only one using the search for multiple fields by now. What do you think about this change?

@MRichards99
Copy link
Contributor

I've just tested this change on DataGateway API and I have no problems with this. The API doesn't convert the list of results into a set so we won't explicitly use this feature, although I can see why it is a good thing. More importantly, it doesn't break anything on the API! I'm happy for you to put this into 0.18.1.

@RKrahl
Copy link
Member Author

RKrahl commented Apr 13, 2021

Yes, that is what I meant by most users will not even notice the difference.

@RKrahl RKrahl merged commit ca172c4 into release/0.18.1 Apr 13, 2021
@RKrahl RKrahl deleted the query-multiple-field-result branch April 13, 2021 17:29
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.

None yet

2 participants