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

Allows SchemaElement instance to use import namespace as targetNamesp… #1095

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ndeniche
Copy link

@ndeniche ndeniche commented Nov 7, 2019

Allows SchemaElement instance to use import namespace as targetNamespace when the attribute is not set.

Prevents the following error message when schema elements do not include targetNamespace attribute:

Target-Namespace "undefined" already in use by another Schema!

If targetNamespace is declared by an import is used in a different schema, it overrides the value for the targetNamespace key.

…ace when the attribute is not set.

### Allows SchemaElement instance to use import namespace as targetNamespace when the attribute is not set.

Prevents the following error message when schema elements do not include targetNamespace attribute:

> Target-Namespace "undefined" already in use by another Schema!
@coveralls
Copy link

coveralls commented Nov 7, 2019

Coverage Status

Coverage decreased (-0.5%) to 94.531% when pulling 79daeb4 on ndeniche:ndeniche_targetNamespace into af93a6f on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented Nov 14, 2019

@ndeniche please fix the build and bring coverage back up.

@ndeniche
Copy link
Author

Same as the other added feature. All of the new code is covered. Should I improve coverage by adding tests to other features?

@jsdevel
Copy link
Collaborator

jsdevel commented Nov 18, 2019

@ndeniche if coverage drops on this build, that means that added code has decreased it right?

@barboni
Copy link
Contributor

barboni commented Dec 11, 2019

Screenshot 2019-12-11 at 10 42 37
This really doesn't make much sense. The overall coverage dropped, but the coverage of each file increased?! 😲

@sfariaNG
Copy link

Was the test coverage on this resolved? Can it be merged soon?

@jsdevel
Copy link
Collaborator

jsdevel commented Feb 10, 2020

@barboni it could be the case that this fix now makes other code unreachable: code who's past assumptions are no longer valid with this fix. can you look into it?

@nicolasjolinUQAM
Copy link

nicolasjolinUQAM commented Jul 21, 2020

I have the same problem as ndeniche and I don't have control over the WSDL. A fix would be appreciated.

@jsdevel
Copy link
Collaborator

jsdevel commented Jul 21, 2020

as long as coverage doesn't dip, we can merge the fix.

@nicolasjolinUQAM
Copy link

It would be kindly appreciated !

@jsdevel
Copy link
Collaborator

jsdevel commented Jul 21, 2020

well, someone has to add a test. feel free to plagaraize this in a new pr with a test

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.

6 participants