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

Issue4 import emmo-inferred from official repo #5

Merged
merged 7 commits into from
Dec 16, 2020

Conversation

francescalb
Copy link
Contributor

@francescalb francescalb commented Oct 30, 2020

Now importing emmo-1.0.0-alpha2 as emmo-inferred directly from official repo instead of including emmo a submodule.

Fixes #4

@francescalb
Copy link
Contributor Author

I have removed emocheck in the workflow for now as it fails. Please comment on this.

Copy link
Contributor

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Why do we not run emmocheck on this now?

.github/workflows/ci_emmocheck.yml Outdated Show resolved Hide resolved
@CasperWA
Copy link
Contributor

CasperWA commented Nov 12, 2020

I have removed emocheck in the workflow for now as it fails. Please comment on this.

Ah, this is why.
But why does it fail?

From this check run it seems physicalDimension is missing in various lattice vectors?

@CasperWA
Copy link
Contributor

CasperWA commented Dec 16, 2020

@francescalb This is fine now and can be merged, right?

Or do you want to close it in favor of #6?
Personally, I would probably hold off merging #6 and instead merge this to keep development possibilities open, while holding off updating to Turtle until it is implemented in the necessary tools.

@francescalb
Copy link
Contributor Author

@francescalb This is fine now and can be merged, right?

Or do you want to close it in favor of #6?
Personally, I would probably hold off merging #6 and instead merge this to keep development possibilities open, while holding off updating to Turtle until it is implemented in the necessary tools.

I think we can merge it, but it is blocked?

@CasperWA
Copy link
Contributor

Try turning emmocheck back on and let's see what happens 👍

@francescalb
Copy link
Contributor Author

Try turning emmocheck back on and let's see what happens 👍

Had to fix the import and skip test_quantity_dimension to have it work.

Try turning emmocheck back on and let's see what happens 👍

Added physicalDimensions and it should work now. @CasperWA

Copy link
Contributor

@CasperWA CasperWA left a comment

Choose a reason for hiding this comment

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

Thanks for enabling emmocheck and making it work again @francescalb !
This is good to go from my end now 👍 🚀

@francescalb francescalb merged commit 9b8b74c into master Dec 16, 2020
@francescalb francescalb deleted the issue4-import-emmo-inferred-official-repo branch December 16, 2020 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants