search

datalad / datalad-catalog

Create a user-friendly data catalog from structured metadata
https://datalad-catalog.netlify.app
MIT License
15 stars 11 forks source link

Fix circular import error with loaded datalad-next #463

Closed christian-monch closed 5 months ago

christian-monch commented 5 months ago

This PR fixes a circular import error that occurs when datalad-next is loaded and tests are patched. The error is described in PR datalad-next #716.

The reason for the import error is that importing datalad_catalog.add from the module-level context in datalad_catalog.tests.test_add, will import datalad_next, which will in turn import certain tests, which import from datalad.api, which will trigger the extension-api generation process, which tries to import and process datalad_catalog.add again (which is not yet completely loaded).

netlify[bot] commented 5 months ago

Deploy Preview for datalad-catalog canceled.

Name Link
Latest commit d54d3e362190e3abb8783e1a346735abcd9d239c
Latest deploy log https://app.netlify.com/sites/datalad-catalog/deploys/6655d3912cea9800086e460e
codecov-commenter commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 76.92%. Comparing base (3f32e6b) to head (d54d3e3). Report is 12 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #463 +/- ## ========================================== - Coverage 83.50% 76.92% -6.58% ========================================== Files 43 45 +2 Lines 2897 3124 +227 ========================================== - Hits 2419 2403 -16 - Misses 478 721 +243 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jsheunis commented 5 months ago

I'm happy with merging this, even if it ends up being a temporary fix. So @christian-monch are you still planning to add a long-term fix to datalad-next, or should I go ahead and merge?

christian-monch commented 5 months ago

I'm happy with merging this, even if it ends up being a temporary fix. So @christian-monch are you still planning to add a long-term fix to datalad-next, or should I go ahead and merge?

I was working on a long-term fix in the last days: https://github.com/datalad/datalad-next/pull/717. It is more or less ready and should solve the problem properly.

Will test with datalad-catalog tomorrow.

christian-monch commented 5 months ago

I successfully ran the datalad-catalog tests with https://github.com/datalad/datalad-next/pull/717. Will close this PR, once https://github.com/datalad/datalad-next/pull/717 is merged and released, the tests should all pass.