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

To dts #126

Merged
merged 26 commits into from
Oct 25, 2019
Merged

To dts #126

merged 26 commits into from
Oct 25, 2019

Conversation

sonofmun
Copy link
Contributor

No description provided.

Bumped to version 1.0.3
* Refactored __transform (flask_nemo.__init__) to _transform
* Changed flask.ext.nemo to flask_nemo
* Updated CHANGES.md and setup.py
* Bumped to version 1.0.3
@sonofmun sonofmun requested review from PonteIneptique and removed request for PonteIneptique October 22, 2019 09:23
@sonofmun
Copy link
Contributor Author

@PonteIneptique I don't need a review here yet. But the tests that are not passing now have to do with fetching annotations. And I am not sure precisely what has changed and where. I will keep researching, but if you have a tip where to look, I would appreciate it.

@sonofmun
Copy link
Contributor Author

sonofmun commented Oct 22, 2019

Perhaps a bit more explanation here. The source of the difference is from https://github.com/Capitains/MyCapytain/blob/3ff0f27e514f5895d4f83dbe294d4035a9e4dc53/MyCapytain/resources/texts/local/capitains/cts.py#L235. This used to return an empty list but now it returns an error.
But even if I catch the error here

(instead of just checking if the list of reffs is empty), then the TestSimpleQuery tests e.g.,
self.assertEqual(annotations[0], self.four, "Deepest node should match as well")
still don't pass because the annotations is actually an empty list and thus annotation[0] throws an index error.

@PonteIneptique
Copy link
Member

Matt, care to have a call this afternoon ?

@PonteIneptique
Copy link
Member

If @balmas is available as well, that would be great.
I think we should drop annotations and other stuff, this might make your life easier.

@sonofmun
Copy link
Contributor Author

Sure. I am available any time between 2pm and 5pm (so 7-10am eastern time).

@sonofmun
Copy link
Contributor Author

FYI, with MyCapytain 3.0.0, the dts-draft-1 branch of Nautlius, and the to_DTS branch of flask-nemo, all of my unit tests work on formulae-capitains-nemo and everything seems to work locally as well.

@sonofmun
Copy link
Contributor Author

I have solved the problem. The difference was that MyCapytain.resources.texts.local.capitains.cts.getValidReff returned simply a list of the passages before 3.0.0 (e.g., ['1.1', '1.2'...]) whereas now it returns a list of CtsReferenceSets, which are tuples. So that means that instead of a list of strings, what is returned is a list of tuples. And so that means that the lines that actually build the annotation list

annotations.extend([
annotation
for annotation in self.annotations
# Exact match
if (str(objectId), subreference) == (annotation.target.objectId, annotation.target.subreference) or
# Deeper match
len(ref_in_range.intersection(annotation.target.expanded)) > 0
])
would not match anything because ref_in_range.intersection(annotation.target.expanded) is never true (ref_in_range is made up of tuples and annotation.target.expanded of strings.
I will push a (very small) change here and then you @PonteIneptique can decide whether this makes sense or not.

@PonteIneptique
Copy link
Member

I was actually having a look at it, and I am still not sure to understand why the first iteration on self.four yields 1.pr.1 as a target... Waiting for your commit then !

All tests now pass with MyCapytain 3.0.0 and Nautilus dts-draft-1
@PonteIneptique
Copy link
Member

I think what you meant is getValidReff returns now CtsReferenceSet which are list of CtsReference which are tuple ?

@sonofmun
Copy link
Contributor Author

That is correct. That is what I meant.

@PonteIneptique
Copy link
Member

Have you tried to browse your current website using this version of nemo ?

@sonofmun
Copy link
Contributor Author

I have. And everything works.

@PonteIneptique
Copy link
Member

Ok, let me do some cosmetic change for one thing or two

@coveralls
Copy link

coveralls commented Oct 22, 2019

Coverage Status

Coverage increased (+0.004%) to 98.494% when pulling d178c77 on to_DTS into 2e87aa2 on dev.

@sonofmun
Copy link
Contributor Author

It is still testing 3.4.5?

@PonteIneptique
Copy link
Member

Yup and it should not. Let me do it ;)

@sonofmun
Copy link
Contributor Author

OK.

@sonofmun
Copy link
Contributor Author

sonofmun commented Oct 22, 2019

Coverage goes down because the line that I left in query/interface.py for backward compatibility is not tested.

if len(reffs) == 0:
break

Perhaps it can be removed since I have put MyCapytain>=3.0.0 in the requirements.txt?

@PonteIneptique
Copy link
Member

Oh and maybe bump the Minor version :')

@sonofmun
Copy link
Contributor Author

Now you tell me.

@sonofmun
Copy link
Contributor Author

Should it be 1.1 instead of 1.0.4?

@PonteIneptique
Copy link
Member

Yup, 1.1.0 is what I meant with minor ;)

@sonofmun
Copy link
Contributor Author

I am not sure about the terminology there.
BTW, you know that this will be a breaking change, right?

@sonofmun
Copy link
Contributor Author

The description of Nemo in setup.py is "Flask Extension to browse CTS Repository". Should I change this? Perhaps, "Flask Extension to browse a CapiTainS-compliant Repository"?

@PonteIneptique
Copy link
Member

Good catch. Yes :)

flask_nemo/chunker.py Outdated Show resolved Hide resolved
flask_nemo/query/interface.py Outdated Show resolved Hide resolved
flask_nemo/__init__.py Show resolved Hide resolved
CHANGES.mD Outdated Show resolved Hide resolved
@PonteIneptique
Copy link
Member

Looks good to me !

@sonofmun sonofmun merged commit 982df8e into dev Oct 25, 2019
@sonofmun sonofmun deleted the to_DTS branch October 25, 2019 10:46
@sonofmun
Copy link
Contributor Author

Should we PR to master as well and do a 2.0.0 release?

@PonteIneptique
Copy link
Member

PonteIneptique commented Oct 25, 2019 via email

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