Skip to content

Commit

Permalink
improved hasExtension method and updated tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@Onur101010
Onur101010 committed May 24, 2020
1 parent 27eda05 commit b8c907e
Show file tree
Hide file tree
Showing 3 changed files with 5 additions and 8 deletions.
2 changes: 1 addition & 1 deletion core/src/org/sbml/jsbml/AbstractSBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1841,7 +1841,7 @@ public int hashCode() {
*/
@Override
public boolean hasExtension(String extName) {
return getModel().getExtension(extName) != null;
return this.getExtension(extName) != null;
}

/*
Expand Down
2 changes: 1 addition & 1 deletion core/src/org/sbml/jsbml/SBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -699,7 +699,7 @@ public List<String> filterCVTerms(CVTerm.Qualifier qualifier, boolean recursive,
public int hashCode();

/**
* Returns {@code true} if the given extension exists in the model
* Returns {@code true} if the given extension exists in this {@link SBase} instance.
*/
public boolean hasExtension(String extName);

Expand Down
9 changes: 3 additions & 6 deletions core/test/org/sbml/jsbml/test/sbml/TestSBase.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1355,8 +1355,7 @@ private String stripIndentation(String notesString) {
}

/**
* Verifies behavior of checking the extension when also
* an other model is involved.
* Verifies behavior of checking the specific extensions.
*/
@Test public void test_SBase_hasExtension() {

Expand All @@ -1377,13 +1376,11 @@ private String stripIndentation(String notesString) {
Dimension dim = arraysModel.createDimension();
dim.addExtension(ArraysConstants.shortLabel, clonedArraysModel);

//dim is using different model for its extension so it is separated
//from the other packages and therefore should not behave the same
assertTrue(sm1.hasExtension(CompConstants.shortLabel) == true);
assertTrue(sm1.hasExtension(LayoutConstants.shortLabel) == true);
assertTrue(sm1.hasExtension(LayoutConstants.shortLabel) == false);
assertTrue(sm1.hasExtension(ArraysConstants.shortLabel) == false);

assertTrue(layout.hasExtension(CompConstants.shortLabel) == true);
assertTrue(layout.hasExtension(CompConstants.shortLabel) == false);
assertTrue(layout.hasExtension(LayoutConstants.shortLabel) == true);
assertTrue(layout.hasExtension(ArraysConstants.shortLabel) == false);

Expand Down

3 comments on commit b8c907e

@draeger
Copy link
Member

Choose a reason for hiding this comment

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

Much better, thanks!

@niko-rodrigue
Copy link
Member

Choose a reason for hiding this comment

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

@draeger @Onur101010 It would be good to let a bit more time to react, in particular when doing changes to the core jsbml classes like SBase and AbstractSBase. And first put what you plan to do on pivotal a little bit before.
Also after each changes, in particular when touching the core, you need to run all the jsbml tests. That is not enforced at the moment but it is important to do it. In this case, you would have noticed that the tests where not compiling.
Last, linked giving a bit more time to react, the method you have added already exist: org.sbml.jsbml.AbstractSBase.isSetPlugin(String). We can add another method, that's fine but I will call org.sbml.jsbml.AbstractSBase.isSetPlugin(String) from hasExtension as the implementation is better there and like this we have only one location to maintain.

@draeger
Copy link
Member

@draeger draeger commented on b8c907e Jun 2, 2020

Choose a reason for hiding this comment

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

Thanks, @niko-rodrigue for this important hint! As a consequence, we should implement continuous integration and testing with Travis upon push.

However, please note that the method isSetPlugin does not deliver the correct result at the moment (see #216).

Please sign in to comment.