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

Eliminate make_iter by eliminating the reason for it #1154

Closed
Kelketek opened this issue Dec 30, 2016 · 6 comments
Closed

Eliminate make_iter by eliminating the reason for it #1154

Kelketek opened this issue Dec 30, 2016 · 6 comments
Labels
devel-implemented Already finished, usually in a branch not yet merged. task A bigger, more general work plan for a bigger issue.

Comments

@Kelketek
Copy link
Contributor

Kelketek commented Dec 30, 2016

Brief summary of issue / Description of requested feature:

Several database querying methods can return either a single object or a list of objects depending on how many are returned. This creates issues when trying to reason about the types returned by the function, and requires something like make_iter to fix it, but it does not truly do so. make_iter creates more boilerplate than it removes, and there are more clear ways to do the same thing.

Steps to reproduce the issue / Reasons for adding feature:

Take this example code:

my_tags = self.tags.get(category='tag')

my_tags could be:

  1. None
  2. A tag object
  3. An empty list

These are three different types, all of which must be handled. Here's some boilerplate code that would handle it:

if my_tags:
    my_tags = make_iter(my_tags)
else:
    my_tags = []

At this point, I can now safely iterate over the list. If I just wanted the first item, I could do:

if my_tags:
    my_tags = make_iter(my_tags)[0]

You will note, however, that this precisely undoes and redoes what the special casing at the end of get methods tries to solve, namely, to only send back one object if it's found. The alternative is to do something like this:

Error output / Expected result of feature

My preference would be to have two methods where one is currently being used, both with different expectations on what's needed.

Extra information, such as Evennia revision/repo/branch, operating system and ideas for how to solve / implement:

One could be filter or query or something, and it would imply that a list is always returned. The other would be get, which would just be a wrapper around the other method where it tries to grab a single item, and raises an error if there's more than one or if there isn't any.

@Kelketek Kelketek added the task A bigger, more general work plan for a bigger issue. label Dec 30, 2016
@BlauFeuer
Copy link
Contributor

It should be sufficient as you say, when only one element is expected, to return only that one element or raise an exception -- or call another method and always get a list with none, one, or many elements whereby instead of coding to process the possible data types, the only processing needed is to count the elements of the returned list before operating on it.

@Griatch
Copy link
Member

Griatch commented Jan 24, 2017

While I agree with the general notion of this, it is a big enough API change that it should go into a future dev branch with other API changes building up to Evennia 0.7.

@Griatch Griatch added this to TODO in Evennia 0.7 Feb 10, 2017
@Griatch Griatch added the devel label Feb 10, 2017
@Griatch Griatch closed this as completed Feb 10, 2017
@Griatch Griatch reopened this Feb 10, 2017
@Griatch Griatch moved this from TODO to In progress on devel branch in Evennia 0.7 Jul 21, 2017
@Griatch Griatch moved this from In progress on devel branch to TODO in Evennia 0.7 Aug 20, 2017
@Griatch
Copy link
Member

Griatch commented Aug 20, 2017

I wonder if it may be better to, rather than have two methods with different expected results, just have one and always return a list from it with 0, 1 or more elements. Since that's similar to our search_* methods, this might help to homogenize the API. It would be no bigger change than introducing a completely new method and change get to be unusable if expecting more than one result.

@Griatch Griatch moved this from TODO to In progress on devel branch in Evennia 0.7 Aug 20, 2017
Griatch added a commit that referenced this issue Aug 27, 2017
See #1154. In the end I didn't modify the Attributehandler and
TagHandler like this, instead I added the `return_list` argument
for cases when one wants a guaranteed return.
@Griatch
Copy link
Member

Griatch commented Aug 27, 2017

@Kelketek I reworked handlers a bit on devel. In the end I opted not to change the current way TagHandler.get and AttributeHandler.get works but instead I added a return_list boolean argument to each. When this is set you will always get a list back from these methos even if it is empty (your default return will also be wrapped in a list). I tried just plain changing to always return lists and it's frankly too much of a chore for attributes - you end up having to check the list instead, most often returning the first element. While returning a list might be more reasonable for tags, I wanted the api between the two to be consistent, so used the return_list boolean there too.

@Griatch
Copy link
Member

Griatch commented Aug 27, 2017

This will close when devel merges.

@Griatch Griatch added the devel-implemented Already finished, usually in a branch not yet merged. label Aug 27, 2017
@Griatch Griatch moved this from In progress on devel branch to Done on devel branch in Evennia 0.7 Aug 27, 2017
@Griatch
Copy link
Member

Griatch commented Sep 20, 2017

Closed with the merger of devel branch.

@Griatch Griatch closed this as completed Sep 20, 2017
@Griatch Griatch moved this from Todo to Finished in API expansion milestone Sep 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devel-implemented Already finished, usually in a branch not yet merged. task A bigger, more general work plan for a bigger issue.
Projects
No open projects
Evennia 0.7
Done on devel branch
Development

No branches or pull requests

3 participants