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

Move metric alias details from MetricSpecPattern to ResolverInputForMetric #1597

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

courtneyholcomb
Copy link
Contributor

This addresses some feedback that did not get addressed before merging this PR - specifically:

Since the alias in this case is describing user inputs, can it be added as a field to ResolverInputForMetric?

@serramatutu
Copy link
Contributor

serramatutu commented Jan 15, 2025

Beginner question: why is it better for the alias to live in ResolverInputForMetric instead of MetricSpecPattern? Is it simply a convention that all user input should live there (in resolver input classes in general) and we want to keep this clear separation? Or is there something more?

@courtneyholcomb
Copy link
Contributor Author

Beginner question: why is it better for the alias to live in ResolverInputForMetric instead of MetricSpecPattern? Is it simply a convention that all user input should live there (in resolver input classes in general) and we want to keep this clear separation? Or is there something more?

@serramatutu It's just a logical separation to be more consistent about what lives where. Spec patterns are meant to match user inputs to available specs. Since the alias is irrelevant to what specs metrics are available for a given query, it doesn't make much sense for it to live there.

@courtneyholcomb courtneyholcomb merged commit 78caba6 into main Jan 16, 2025
37 of 39 checks passed
@courtneyholcomb courtneyholcomb deleted the court/alias-cleanup branch January 16, 2025 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants