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

tests: mov query_execution snapshot to a separate folder #957

Conversation

CommanderStorm
Copy link
Contributor

@CommanderStorm CommanderStorm commented Sep 29, 2024

I read your oppinion in #951 (comment) as a preference for atomic, squashed PRs.
This PR moves the files as required/prefered by #951
=> the other PR can now be squashed as prefered

Likely needs to be rebased onto #956

@CommanderStorm CommanderStorm changed the title tests: moved the query execution snapshot to a separate place tests: mov query_execution snapshot to a separate folder Sep 29, 2024
@obi1kenobi
Copy link
Owner

Could you say a bit more about why moving these files is advantageous? Right now it seems a bit arbitrary.

Since this move would also break our existing contributing advice and workflow, I'm a bit hesitant to commit to it without understanding the motivation better first.

P.S.: We have a Discord server for coordinating contributions, you're welcome to join if you'd like (the invite is good for 7 days): https://discord.gg/vf6AGXQU

@CommanderStorm
Copy link
Contributor Author

Having this be in its own directory makes sense in my mind given the number of files that come from this check. At least that would be the difference between the whiteness and cmd snapshots.

=> This is more of a "this seems like it fits into the existing art" PR, as this seems a bit different from the manual vs the insta based snapshots.

If this breaks other PRs I would be happy to provide PRs against them to fix this so that the contributiotors in said PRs don't have to rerun cargo test.

@obi1kenobi
Copy link
Owner

I think merging this on its own is too risky, unfortunately. As-is, it results in a broken repo state: the contributing docs reference files that don't exist and provide incorrect steps to resolve them. The snapshot files are in an unfamiliar format whose .snap extension doesn't even offer syntax highlighting. Etc.

I'd actually much prefer to squash-merge this as part of #951, so that at all points the main branch has a state that makes sense to build and contribute from. And if #951 ever needs to be reverted, it can be reverted as one unit instead of needing to revert separate commits.

The only changes I wanted to remove from #951 are the "unrelated fixes" you felt were necessary, since they were tangential to the point of #951. So I don't think this PR is needed.

@CommanderStorm
Copy link
Contributor Author

don't think this PR is needed.

Fair enough, let's continue in #951, sorry for the misunderstanding

@obi1kenobi
Copy link
Owner

No worries at all, thank you for contributing!

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