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 RebenchLog case; error on missing gauge adapter #200

Closed
wants to merge 1 commit into from

Conversation

raehik
Copy link
Contributor

@raehik raehik commented Jan 24, 2023

Addresses #199 . Fixes the case of the RebenchLog gauge adapter and adds some notes on case sensitivity in relevant places. In the gauge adapter resolver, we emit an error message for missing gauge adapters instead of leaving it to the Python stack trace.

@raehik
Copy link
Contributor Author

raehik commented Jan 24, 2023

I left a TODO in the code: execution should exit as soon as reasonable if no gauge adapter can be decided on. There's no precedent in the code, and I'm not sure what needs cleaning up. Is sys.exit(1) fair game?

Comment on lines +407 to +414
# obtain gauge adapter module from provided name or die trying
gauge_adapter_name = run_id.get_gauge_adapter_name()
gauge_adapter = self._get_gauge_adapter_instance(gauge_adapter_name)
if gauge_adapter is None:
run_id.fail_immediately()
msg = "{ind}Couldn't find gauge adapter: %s\n" % gauge_adapter_name
self.ui.error(msg, run_id)
# TODO exit early here (how?)
Copy link
Owner

Choose a reason for hiding this comment

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

If we decided that #202 is what we want, then I would suggest to still have this error reporting. Though, I probably would move it into _get_gauge_adapter_instance.

Depending on your preferences, I can include the change into #202.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#202 is best! Please add it there.

@smarr
Copy link
Owner

smarr commented Jan 28, 2023

if no gauge adapter can be decided on

I do like that you fail the run_id. I'd still let the rest continue. You reported an error to the user, which is good. But so far, I refrained from any hard exits.

What might be annoying is to get a hundred error messages that are the same. Perhaps we want to keep a dictionary of adapter names we already warned about.

@raehik
Copy link
Contributor Author

raehik commented Jan 28, 2023

I think this should be dropped in favour of #202 (and the docs tweaks/error messages can be added on over there). I'll give it a close for now.

@raehik raehik closed this Jan 28, 2023
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

2 participants