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

Remove collections test module #885

Closed
wants to merge 1 commit into from
Closed

Conversation

courtneyholcomb
Copy link
Contributor

Description

Small thing for internal scripts! Adding this collections module to the test directory overrode our ability to use the built-in Python collections module from test. This broke at least the ability to run metricflow/test/generate_snapshots.py, and likely any other scripts in the test directory. As far as I can tell, we don't need this to be a functioning module (nothing imports from it) so we can just remove the __init__.py to resolve this.

Copy link
Contributor

@tlento tlento left a comment

Choose a reason for hiding this comment

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

This seems fine, but maybe we should just rename the collections directory here and in the main metricflow sub-directory instead. Given what's in there collections is a strange name, but I don't know what @plypaul has planned in upcoming PRs.

@courtneyholcomb
Copy link
Contributor Author

This seems fine, but maybe we should just rename the collections directory here and in the main metricflow sub-directory instead. Given what's in there collections is a strange name, but I don't know what @plypaul has planned in upcoming PRs.

I think the intent was to copy the same file structure that exists in metricflow/ into metricflow/test/ for the mocks! @plypaul do you have a different preference for how to resolve this? Happy to change the approach if so

@tlento
Copy link
Contributor

tlento commented Nov 17, 2023

I think the intent was to copy the same file structure that exists in metricflow/ into metricflow/test/ for the mocks! @plypaul do you have a different preference for how to resolve this? Happy to change the approach if so

Right, I'm suggesting renaming both directories to preserve the parallelism (and remove the possibility of ambiguity in from collections import.... inside of MetricFlow proper). But naming is hard.

@plypaul
Copy link
Contributor

plypaul commented Nov 18, 2023

I didn't realize that there was a naming collision - in which case, we should definitely rename it. Let me put in a commit since it'll make it easier to manage my stack.

@courtneyholcomb
Copy link
Contributor Author

Closing this, @plypaul will put up a different fix.

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