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

fix: appmap.Recording is available even when APPMAP=false #327

Merged
merged 1 commit into from
May 30, 2024

Conversation

zermelo-wisen
Copy link
Collaborator

@zermelo-wisen zermelo-wisen commented May 29, 2024

Fixes #326.

  • Adds NoopRecording class with the same interface as Recording.
  • Exports NoopRecording as Recording when APPMAP is not set to true to prevent "ImportError: cannot import name 'Recording' from 'appmap' ..." in client code.
  • Extends smoketest.sh to ensure that client code using appmap.Recording does not fail when APPMAP=false.

@zermelo-wisen zermelo-wisen force-pushed the fix/import-recording-when-not-enabled branch 4 times, most recently from b58c807 to 325a58b Compare May 29, 2024 12:42
@zermelo-wisen zermelo-wisen force-pushed the fix/import-recording-when-not-enabled branch from 325a58b to 6affac6 Compare May 30, 2024 05:03
Copy link
Collaborator

@apotterri apotterri 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 this, looks good. One minor change and it'll be ready to go.

ci/smoketest.sh Outdated
@@ -27,3 +27,22 @@ else
find $PWD
exit 1
fi

# Ensure that client code using appmap.Recording does not fail when APPMAP=false
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can get moved up before https://github.com/getappmap/appmap-python/pull/327/files#diff-ba2ff002750238ee03b3467fd4400804141c8aa2606bdd4263f0a3636bb120c1R12 to avoid flipping APPMAP on and off.

This file is starting to get kind of complicated. Please move this test into a separate function. Thanks!

@zermelo-wisen zermelo-wisen force-pushed the fix/import-recording-when-not-enabled branch from 6affac6 to d7c522e Compare May 30, 2024 09:32
@zermelo-wisen zermelo-wisen force-pushed the fix/import-recording-when-not-enabled branch from d7c522e to 6bb7687 Compare May 30, 2024 09:37
Copy link
Collaborator

@apotterri apotterri left a comment

Choose a reason for hiding this comment

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

Thanks, good to go.

@apotterri apotterri merged commit 25130d2 into master May 30, 2024
2 checks passed
@apotterri apotterri deleted the fix/import-recording-when-not-enabled branch May 30, 2024 11:21
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.

appmap.Recording is available even when APPMAP=false
2 participants