Skip to content

Mention cargo-diet and sort in README #1449

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 2 commits into from
Jul 11, 2024

Conversation

cruessler
Copy link
Contributor

This PR mentions 2 issues I encountered when I ran just test check-size for the first time.

  • I did not have cargo-diet installed, so I thought it might be a good idea to mention it.

  • I did not have LC_ALL set in my environment which made one of the tests fail with the following diff:

    < ./a-non-bare-repo-with-extension.git/.git
    4a4
    > ./a-non-bare-repo-with-extension.git/.git
    8d7
    < ./no-origin/.git
    9a9
    > ./no-origin/.git
    11d10
    < ./origin-and-fork/.git
    12a12
    > ./origin-and-fork/.git
    14,15c14,15
    < ./special-origin/.git
    < ./special-origin/a
    \ No newline at end of file
    ---
    > ./special-origin/a
    > ./special-origin/.git
    \ No newline at end of file
    

    Notice that dotfiles and non-dotfiles were sorted in a way that did not match the snapshot. One could also individually set LC_ALL=C for all or just the failing invocations of sort in shell scripts.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the fly-by fixes!

Now that I see it, I think even though cargo diet can be mentioned, the part that says running should just be just test, check-size I haven't run myself in a long time, it's very occasional only.

With LC_ALL, I think it would be better to at least put it into the top-level journey test, I wasn't aware that the sort-order is dependent on the locale.

Does that make sense?

Turn comment into code in `journey.sh`.
@cruessler
Copy link
Contributor Author

That makes a lot of sense! I’ve updated the PR accordingly.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Perfect, thanks a lot :)!

@Byron Byron merged commit af6446a into GitoxideLabs:main Jul 11, 2024
19 checks passed
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