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

Adds url to automatic definition writing #361

Merged
merged 2 commits into from
Jul 4, 2024
Merged

Conversation

domna
Copy link
Collaborator

@domna domna commented Jul 2, 2024

This adds that entry/definition/@url is automatically set to the nexus definitions repo. We also set this in the root attribute NexusRepository but @RonHildebrandt and I thought it would be reasonable to also automatically add it to the definition.

In NXentry the attribute is actually uppercase (@URL) so we might need to change this accordingly. @RonHildebrandt this should then also be changed in NXraman and any appdefs this is required in.

@domna domna self-assigned this Jul 2, 2024
@domna domna requested a review from RonHildebrandt July 2, 2024 13:06
@coveralls
Copy link

coveralls commented Jul 2, 2024

Pull Request Test Coverage Report for Build 9761543512

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.916%

Totals Coverage Status
Change from base Build 9742172344: 0.0%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9761543512

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.916%

Totals Coverage Status
Change from base Build 9742172344: 0.0%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

Copy link
Contributor

@RonHildebrandt RonHildebrandt left a comment

Choose a reason for hiding this comment

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

This sounds good. Think it is generally good to add this in a NeXus file itself, and not the part which is added from pynxtools. (e.g. if NeXus file is created without pynxtools).

@domna
Copy link
Collaborator Author

domna commented Jul 2, 2024

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

@lukaspie
Copy link
Collaborator

lukaspie commented Jul 2, 2024

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

I have a branch FAIRmat-NFDI/pynxtools-xps#67 that updates the test files (and which works with this branch here, check the pynx compatibility action), but then the pytest actions in pynxtools-xps are failing (because it checks against released pynxtools). What would be the best strategy here? Merge this branch here with the plugin tests failing and then we do the changes in the plugin repos?

@domna
Copy link
Collaborator Author

domna commented Jul 2, 2024

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

I have a branch FAIRmat-NFDI/pynxtools-xps#67 that updates the test files (and which works with this branch here, check the pynx compatibility action), but then the pytest actions in pynxtools-xps are failing (because it checks against released pynxtools). What would be the best strategy here? Merge this branch here with the plugin tests failing and then we do the changes in the plugin repos?

Yes, I would say so. We should probably coordinate the merges and then do it quickly after each other

@lukaspie
Copy link
Collaborator

lukaspie commented Jul 2, 2024

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

I have a branch FAIRmat-NFDI/pynxtools-xps#67 that updates the test files (and which works with this branch here, check the pynx compatibility action), but then the pytest actions in pynxtools-xps are failing (because it checks against released pynxtools). What would be the best strategy here? Merge this branch here with the plugin tests failing and then we do the changes in the plugin repos?

Yes, I would say so. We should probably coordinate the merges and then do it quickly after each other

Ok, from my side, it works, so just let me know when you merge this. I guess we would need @RubelMozumder to update his test data first.

@domna
Copy link
Collaborator Author

domna commented Jul 2, 2024

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

I have a branch FAIRmat-NFDI/pynxtools-xps#67 that updates the test files (and which works with this branch here, check the pynx compatibility action), but then the pytest actions in pynxtools-xps are failing (because it checks against released pynxtools). What would be the best strategy here? Merge this branch here with the plugin tests failing and then we do the changes in the plugin repos?

Yes, I would say so. We should probably coordinate the merges and then do it quickly after each other

Ok, from my side, it works, so just let me know when you merge this. I guess we would need @RubelMozumder to update his test data first.

Jep, I also need to update it for mpes. But lets wait and coordinate with @RubelMozumder (probably tomorrow since he has internet issues today)

@RubelMozumder
Copy link
Collaborator

cc @RubelMozumder @lukaspie, you would need to update the stm and xps regression files for this change to pass the tests

I have a branch FAIRmat-NFDI/pynxtools-xps#67 that updates the test files (and which works with this branch here, check the pynx compatibility action), but then the pytest actions in pynxtools-xps are failing (because it checks against released pynxtools). What would be the best strategy here? Merge this branch here with the plugin tests failing and then we do the changes in the plugin repos?

Yes, I would say so. We should probably coordinate the merges and then do it quickly after each other

Ok, from my side, it works, so just let me know when you merge this. I guess we would need @RubelMozumder to update his test data first.

Jep, I also need to update it for mpes. But lets wait and coordinate with @RubelMozumder (probably tomorrow since he has internet issues today)

I update the pynxtools-stm, regarding the PR. @domna you can approve it and merge it. So, you will get any github action failing from plugin_test action.

@domna
Copy link
Collaborator Author

domna commented Jul 3, 2024

We need to use uppercase @URL, otherwise the concept is not recognised. @RonHildebrandt I think you should update this in your NXoptical_spectroscopy definition accordingly. @lukaspie Sorry, I think you need to do another update for your regression file to account for the uppercase notation.

Otherwise, we are just waiting on FAIRmat-NFDI/pynxtools-stm#39 as the last merge which needs to be ready so we can do it quickly after each other.

@coveralls
Copy link

coveralls commented Jul 3, 2024

Pull Request Test Coverage Report for Build 9777818759

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.916%

Totals Coverage Status
Change from base Build 9742172344: 0.0%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

@lukaspie
Copy link
Collaborator

lukaspie commented Jul 3, 2024

We need to use uppercase @URL, otherwise the concept is not recognised. @RonHildebrandt I think you should update this in your NXoptical_spectroscopy definition accordingly. @lukaspie Sorry, I think you need to do another update for your regression file to account for the uppercase notation.

Otherwise, we are just waiting on FAIRmat-NFDI/pynxtools-stm#39 as the last merge which needs to be ready so we can do it quickly after each other.

No worries, I updated the pynxtools-xps branch one more.

@coveralls
Copy link

coveralls commented Jul 4, 2024

Pull Request Test Coverage Report for Build 9790520604

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.916%

Totals Coverage Status
Change from base Build 9790306982: 0.0%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

1 similar comment
@coveralls
Copy link

Pull Request Test Coverage Report for Build 9790520604

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 96.916%

Totals Coverage Status
Change from base Build 9790306982: 0.0%
Covered Lines: 597
Relevant Lines: 616

💛 - Coveralls

@domna domna merged commit 8cd8755 into master Jul 4, 2024
10 checks passed
@domna domna deleted the add-url-to-auto-definition branch July 4, 2024 07:56
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

5 participants