Skip to content

Revert "Fix parsing snapshot file" #89

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

Merged
merged 4 commits into from
Jul 2, 2025

Conversation

NikolasHaimerl
Copy link
Contributor

Reverts #85
The change in schema was a bug that was introduced and will be reverted.
See the conversation here: https://space-meridian.slack.com/archives/C023K7D9GAX/p1751283455545599

@juliangruber juliangruber marked this pull request as draft July 1, 2025 10:31
@juliangruber
Copy link
Member

Converted to draft to prevent accidental early merge after approval

@juliangruber
Copy link
Member

The first commit lgtm, but what about the second one, labeled clippy? ee4cd0b. It removes important stuff

@juliangruber
Copy link
Member

Are you sure this can be merged?

@NikolasHaimerl
Copy link
Contributor Author

The first commit lgtm, but what about the second one, labeled clippy? ee4cd0b. It removes important stuff

After I opened the revert PR clippy was complaining which indicates that in between the firs time the code was changed and now, clippy was reconfigured. This meant I needed to run clippy locally to fix the formatting issues. It seems like the commenting of the run.sh file which I did for verifying the original solution from Srdjan slipped in. I removed this commenting.

@juliangruber
Copy link
Member

For my awareness, what is clippy?

@NikolasHaimerl
Copy link
Contributor Author

For my awareness, what is clippy?

Clippy is a linting tool for rust, similar to how we use prettier in Javascrpt.

@NikolasHaimerl
Copy link
Contributor Author

Are you sure this can be merged?

Yes, I checked it against the current version of the snapshot.
See the conversation here: https://space-meridian.slack.com/archives/C023K7D9GAX/p1751439646372569?thread_ts=1751283455.545599&cid=C023K7D9GAX

@NikolasHaimerl
Copy link
Contributor Author

Converted to draft to prevent accidental early merge after approval

I think this is a repo setting, should we update it to require a PR and approval before merging?

@juliangruber
Copy link
Member

Are you sure this can be merged?

Yes, I checked it against the current version of the snapshot. See the conversation here: https://space-meridian.slack.com/archives/C023K7D9GAX/p1751439646372569?thread_ts=1751283455.545599&cid=C023K7D9GAX

My comment was referring to the clippy commit

@juliangruber
Copy link
Member

Converted to draft to prevent accidental early merge after approval

I think this is a repo setting, should we update it to require a PR and approval before merging?

Not necessarily, making it a draft just prevents any kind of accidental merge situation

@juliangruber
Copy link
Member

For my awareness, what is clippy?

Clippy is a linting tool for rust, similar to how we use prettier in Javascrpt.

Interesting, and it also lints shell files?

@NikolasHaimerl
Copy link
Contributor Author

For my awareness, what is clippy?

Clippy is a linting tool for rust, similar to how we use prettier in Javascrpt.

Interesting, and it also lints shell files?

No, I do not think it lints shell files.

@NikolasHaimerl
Copy link
Contributor Author

Are you sure this can be merged?

Yes, I checked it against the current version of the snapshot. See the conversation here: https://space-meridian.slack.com/archives/C023K7D9GAX/p1751439646372569?thread_ts=1751283455.545599&cid=C023K7D9GAX

My comment was referring to the clippy commit

I see, the clippy commit was aiming at fixing the lining issue that clippy was raising.

@NikolasHaimerl NikolasHaimerl marked this pull request as ready for review July 2, 2025 12:22
@NikolasHaimerl NikolasHaimerl merged commit 1f374b5 into main Jul 2, 2025
5 checks passed
@NikolasHaimerl NikolasHaimerl deleted the revert-85-fix/parsing-snapshot-file branch July 2, 2025 12:23
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