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

Add iati-activity/result/reference and test cases #383

Merged

Conversation

dalepotter
Copy link
Contributor

Fixes #362

@dalepotter dalepotter added this to the 2.03 Release milestone Nov 22, 2017
@dalepotter dalepotter requested a review from a team November 22, 2017 12:09
Copy link
Contributor

@hayfield hayfield left a comment

Choose a reason for hiding this comment

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

One redundant test file and a couple of comments that could be a tad clearer.

<title>
<narrative>Sample text</narrative>
</title>
<reference/><!-- FAIL: Optional `reference` element present, but both the mandatory `code` and `vocabulary` attributes are not present. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

If they are tested individually, this test file is not required.

(and if they aren't tested individually, this test file doesn't check the things that each is required)

...and there's no manner in XSD 1.0 to say that one of a pair of attributes is required https://stackoverflow.com/a/763108

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 1729f95.

<narrative>Sample text</narrative>
</title>
<reference vocabulary="xx" code="xx"><!-- PASS: Optional `reference` element present, with two mandatory attributes. -->
<example-namespace:namespace-element></example-namespace:namespace-element><!-- Optional namespace element included. -->
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be clearer that this is what is being tested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made changes in 59a684c - is it clearer now?

@@ -0,0 +1,33 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? Isn't it covered by the minimal IATI file case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, leave for integration branch as needs to be moved to result level folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in e23a674 anyway!

Each required attribute is tested indivually, so there is no need to test that both are not present. Additionally there is no manner in XSD 1.0 to say that one of a pair of attributes is required.
@dalepotter dalepotter dismissed hayfield’s stale review November 23, 2017 12:14

Required changes made

@dalepotter dalepotter merged commit 4fd1320 into result-element-integration Nov 28, 2017
@hayfield hayfield deleted the 2-03-add-reference-element-in-result branch November 28, 2017 11:53
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