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

DAFFODIL-2200. xs:import problems with xsd files provided in daffodil-lib #269

Merged
merged 1 commit into from
Sep 27, 2019

Conversation

dineshchitlangia
Copy link
Contributor

Jira had reported 2 issues:

  1. schemaLocation attributes should just be the filenames
  2. missing schemaLocation in XMLSchema_for_DFDL.xsd when importing dfdl namespace.

Issue 1 was not seen and probably fixed by another jira in the past.

This PR addresses issue 2.

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

See other comment to fix.

You should squash what will be 3 commits together and then force-push your changes again. That way it will be one single commit when it hits the master.

Thanks

@dineshchitlangia
Copy link
Contributor Author

See other comment to fix.

You should squash what will be 3 commits together and then force-push your changes again. That way it will be one single commit when it hits the master.

Thanks

@mbeckerle Updated the PR as advised. I will recheck after CI run.

@dineshchitlangia
Copy link
Contributor Author

@mbeckerle it looks like for some reason one of the CI phases timed out at Integration test.
Could you help to re-trigger it pls?

@mbeckerle
Copy link
Contributor

Will try again tomorrow. My current attempts to re-try all tests then fail with a git checkout error that makes no sense:
git checkout --progress --force 77f6143
##[error]fatal: reference is not a tree: 77f6143

@stevedlawrence
Copy link
Member

It looks like the GitHub actions CI is failing due to actions/checkout#23. I think we'll just have to rely on TravisCI until this issue is resolved.

Though, it looks like the first part of the bug is still an issue. The dfdlx.xsd file has the following lines:

  <xs:import namespace="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:int" schemaLocation="xsd/dafint.xsd" />
  <xs:import namespace="http://www.ogf.org/dfdl/dfdl-1.0/" schemaLocation="xsd/DFDL_part3_model.xsd" />

The bug report says that the schemaLocation attributes should be this:

  <xs:import namespace="urn:ogf:dfdl:2013:imp:daffodil.apache.org:2018:int" schemaLocation="dafint.xsd" />
  <xs:import namespace="http://www.ogf.org/dfdl/dfdl-1.0/" schemaLocation="DFDL_part3_model.xsd" />

I.e., the "xsd" directory is removed from the schemaLocation. I'd doesn't look like that was fixed as another PR. I think the bug is right and the xsd should be removed.

Also, I recommend changing the commit message to remove the squashed commit messge "fixed broken tag" since the squashed commit doesn't actually fix any broken tags. Also, in the Daffodil project, the standard is to list the bug number at the end of the commit message, so your commit message should look something like this:

xs:import problems with xsd files provided in daffodil-lib

Removed extra whitespace

DAFFODIL-2200

…-lib

xs:import problems with xsd files provided in daffodil-lib

Corrected schemaLocation to remove prefix 'xsd/'
Removed extra whitespaces
Added schemaLocation for dfdl namespace

DAFFODIL-2200
@dineshchitlangia
Copy link
Contributor Author

@stevedlawrence thank you for checking this PR. I have addressed review comments in latest commit.

@codecov-io
Copy link

codecov-io commented Sep 26, 2019

Codecov Report

Merging #269 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #269   +/-   ##
=======================================
  Coverage   80.36%   80.36%           
=======================================
  Files         395      395           
  Lines       23271    23271           
  Branches     2699     2699           
=======================================
  Hits        18701    18701           
  Misses       4570     4570

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5f167e3...6d03397. Read the comment docs.

Copy link
Member

@stevedlawrence stevedlawrence left a comment

Choose a reason for hiding this comment

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

+1 👍 Thanks!

Copy link
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

+1

@mbeckerle mbeckerle merged commit 8569387 into apache:master Sep 27, 2019
@dineshchitlangia
Copy link
Contributor Author

Thank you all for review and guidance on my first contribution to this project!

@dineshchitlangia dineshchitlangia deleted the DAFFODIL-2200 branch September 27, 2019 13: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.

4 participants