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

Dataset class documentation refers to non-existant .graphs() method #495

Closed
ssssam opened this issue Jul 9, 2015 · 9 comments
Closed

Dataset class documentation refers to non-existant .graphs() method #495

ssssam opened this issue Jul 9, 2015 · 9 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request fix-in-progress
Milestone

Comments

@ssssam
Copy link
Contributor

ssssam commented Jul 9, 2015

In a few places the Dataset class docstring refers to a Dataset.graphs() method. But there's no such thing. Should it refer to Dataset.contexts() instead? Or should Dataset.graphs() be added as an alias of Dataset.contexts() ?

@joernhees joernhees added enhancement New feature or request documentation Improvements or additions to documentation labels Jul 9, 2015
@joernhees joernhees added this to the rdflib 4.2.1 milestone Jul 9, 2015
@joernhees
Copy link
Member

i think you're right and it should refer to Dataset.contexts() but it's probably safest to ask @iherman and @gjhiggins again who modified those lines...

@ghost
Copy link

ghost commented Jul 28, 2015

The program semantics will be Ivan's call, my edits were in pursuit of better PEP8 conformance and confined to the syntax level.

@iherman
Copy link
Contributor

iherman commented Aug 1, 2015

I am sorry guys, you are absolutely right; my mistake. I have added context to the Dataset to 'hide' the behaviour of the supertype, but the graphs method should be the obvious method for the user. (Ie, no need to document context, in fact.)

I have added graphs, see PR #504; I let you pull it.

Note that I have simply copied context verbatim (and have tested it locally). I am not sure whether there is a more elegant way of referring to self.context, because self.context is an iterator (using yield). Instead of experimenting, I just made a verbatim copy.

Cc: @gjhiggins, @joernhees

@gromgull
Copy link
Member

gromgull commented Aug 2, 2015

I removed this intentionally in 1ed4feb

as it does exactly what the .contexts method does.

@gromgull
Copy link
Member

gromgull commented Aug 2, 2015

I dont feel very strongly about whether we reintroduce the method or fix the docs, but the method should just call .contexts, not duplicate the code.

@iherman
Copy link
Contributor

iherman commented Aug 2, 2015

@gromgull

see my comment on whether to use graphs() or not.

As for duplication or not: as I said, just saying

def graphs(self,t):
    return self.contexts(t)

does not work because contexts is an iterator. I was not sure what the best way is to do that, maybe

def graphs(self,t):
    for s in self.contexts(s) : yield s

would work, but I did not have the time to test that (thinking about it further this should probably work!). There is probably a standard python idiom for this, but I do not know from the top of my head.

@joernhees
Copy link
Member

i don't understand why just returning wouldn't work?

In [3]: class Foo(object):
   ...:     def gen(self, t):
   ...:         yield 'my generator'
   ...:         yield 'yields'
   ...:         for _ in range(10):
   ...:             yield '12 things'
   ...:     def recall(self, t):
   ...:         return self.gen(t)
   ...:     alias = gen
   ...:

In [4]: foo = Foo()

In [5]: foo.gen(21)
Out[5]: <generator object gen at 0x105a2fe60>

In [6]: foo.recall(21)
Out[6]: <generator object gen at 0x105a2fc30>

In [7]: foo.alias(21)
Out[7]: <generator object gen at 0x105a2fe10>

In [8]: for s in foo.gen(21):
   ...:     print s
   ...:
my generator
yields
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things

In [9]: for s in foo.recall(21):
    print s
   ...:
my generator
yields
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things

In [10]: for s in foo.alias(21):
    print s
   ....:
my generator
yields
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things
12 things

Anyhow, i accidentally didn't refresh these tabs since yesterday and just made Dataset.graphs an alias of Dataset.contexts by graphs = contexts over in #504. That way we don't introduce another level in the call stack just for aliasing, so IMO it's the better way anyhow.

Any objections / anything else to re-introduce?

Otherwise i'd merge #504 soon and we can move on.

@iherman
Copy link
Contributor

iherman commented Aug 2, 2015

For some reasons it did not work for me yesterday; but then maybe (probably) I did a mistake. Your fix is obvious (should have thought about it), so please merge and close it!

Thanks

@joernhees
Copy link
Member

;) done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request fix-in-progress
Projects
None yet
Development

No branches or pull requests

4 participants