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

[WIP] Issue 424/add saferepr #460

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

anusha-shekhar
Copy link

@anusha-shekhar anusha-shekhar commented Dec 13, 2023

Description

An error was thrown after enabling tracing if one of the local objects lacked a specific outdated Scikit-Learn object. The issue asked to include error handling when outputting tracing messages for invalid string representations in order to increase the robustness of Pluggy’s error handling mechanism.

Solution

The saferepr module was implemented and adapted to Pluggy, as done in the Pytest repo. Saferepr includes a generous try...except clause along with additional error handling. This ensures that errors do not bubble up.

Steps to test

  • Provide an invalid repr object
  • It should safely handle such a string representation

Link to issue

#424

@anusha-shekhar anusha-shekhar marked this pull request as draft December 13, 2023 04:54
@anusha-shekhar anusha-shekhar changed the title Issue 424/add saferepr [WIP] Issue 424/add saferepr Dec 13, 2023
Comment on lines 463 to 461
# Intentional error to make the repr fail
raise ValueError("Intentional error in he_method2")

Copy link
Member

Choose a reason for hiding this comment

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

This is not an error unable for the particular test

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure I understand the comment here, I am just trying to mock a random error so that we can test the saferepr implementation.

@anusha-shekhar anusha-shekhar force-pushed the issue-424/add-saferepr branch 3 times, most recently from 35ede04 to e6d20c6 Compare December 14, 2023 04:11
@anusha-shekhar
Copy link
Author

Hi! It seems that some of the code coverage pipeline is inaccurate. It is counting empty lines as requiring test coverage (please see screenshot provided)

Screenshot 2023-12-13 at 11 36 23 PM

@@ -456,7 +457,7 @@ def _add_hookimpl(self, hookimpl: HookImpl) -> None:
self._hookimpls.insert(i + 1, hookimpl)

def __repr__(self) -> str:
return f"<HookCaller {self.name!r}>"
Copy link
Member

Choose a reason for hiding this comment

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

Why blindly replace all usages?

Copy link
Member

@RonnyPfannschmidt RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

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

as far as i can tell this pull request blindly replaces calls, but does not address the casting issue in the tracing code at all

based on the code as im looking at now, a simplified try/except based "safe str" is needed

also a test using tracing that traces a broken object as input is needed,

hook_impl = HookImpl(plugin, plugin_name, example_function, hook_impl_opts)

# Verify attributes are set correctly
assert hook_impl.function == example_function
Copy link
Member

Choose a reason for hiding this comment

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

that type of test adds no value as far as can tell - lets keep experiments out of the codebase


# Verify __repr__ method
expected_repr = (
f"<HookImpl plugin_name={saferepr(plugin_name)}, " f"plugin={saferepr(plugin)}>"
Copy link
Member

Choose a reason for hiding this comment

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

we shouldnt add safe-repr all over the place for everything

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.

2 participants