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

DTS Resolver, available in your closest store #184

Merged
merged 22 commits into from Feb 1, 2019
Merged

Conversation

PonteIneptique
Copy link
Member

This is a complete implementation of the DTS Resolver, even so passage metadata are a bit tricky and not perfectly handled from my point of view. Could be better but I need more use cases.

From MyCapytain.resources.prototypes.text to MyCapytain.resources.prototypes.cts.text :

- CtsNode now PrototypeCtsNode
- CtsPassage now PrototypeCtsPassage
- CtsText now PrototypeCtsText
@PonteIneptique PonteIneptique requested a review from a team October 23, 2018 09:31
@PonteIneptique
Copy link
Member Author

Oh, and going in, I did some cleaning. Mostly I intent to remove any misused self.__privatevar__

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 93.94% when pulling 4ad13e0 on dts-resolver into 2ea4c8b on dev.

@coveralls
Copy link

coveralls commented Oct 23, 2018

Coverage Status

Coverage increased (+0.5%) to 93.942% when pulling f532e7d on dts-resolver into 2ea4c8b on dev.

Copy link
Contributor

@sonofmun sonofmun left a comment

Choose a reason for hiding this comment

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

There was a bit here that I do not feel qualified to comment on. And I didn't look at the tests since I don't feel like I have a good enough grasp on the codebase to judge them. But I did make a few suggestions. I am sure I will have more once I try to implement it.

MyCapytain/common/metadata.py Show resolved Hide resolved
if ascendants != 0 and self.updwards[t] is True:
self.updwards[t] = False
for s, p in graph.subject_predicates(object=target):
if desc == 0 and isinstance(s, BNode):
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be if asc == 0 and isinstance(s, BNode): since this if clause is about ascendants as opposed to descendants?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I understand now why it is desc == 0 and not asc (because it is getting the descendants of the subjects in the graph?). But I will leave the comment here just to make sure.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn, you throw some doubts about this. It works so assu,e it's working ok. But honnestly, this is some part of the code where I don't feel too good about my own readability...

MyCapytain/resolvers/dts/api_v1.py Outdated Show resolved Hide resolved
MyCapytain/resolvers/dts/api_v1.py Outdated Show resolved Hide resolved
MyCapytain/resources/collections/dts/_resolver.py Outdated Show resolved Hide resolved
MyCapytain/resources/prototypes/cts/text.py Outdated Show resolved Hide resolved
MyCapytain/resources/prototypes/metadata.py Show resolved Hide resolved
@PonteIneptique
Copy link
Member Author

It is true it needs more docstrings :P

@PonteIneptique
Copy link
Member Author

I thinkit's time

@PonteIneptique PonteIneptique merged commit b11bbf6 into dev Feb 1, 2019
@PonteIneptique PonteIneptique deleted the dts-resolver branch July 1, 2019 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants