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

fix: repoURL matching is case sensitive #61

Merged

Conversation

SIMULATAN
Copy link
Contributor

@SIMULATAN SIMULATAN commented Nov 6, 2024

this change is currently untested, I plan on conducting a full one soon

While testing this awesome project in my own repository, I found that using different casing in the application repoURL results in a confusing error message printing that the repoURL key was missing entirely. We may want to consider printing something more helpful in that case (for example, "Different 'repoURL' under spec.source in file {}")

Additionally, I fixed the debug message printing spec.sources[] as the source even when spec.source is used, which mislead me at the start. (=> rebase)

Copy link
Owner

@dag-andersen dag-andersen left a comment

Choose a reason for hiding this comment

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

Thank you! And thank you for the contribution 🚀
I believe the build is failing because you are missing two &

src/parsing.rs Outdated Show resolved Hide resolved
src/parsing.rs Outdated Show resolved Hide resolved
Co-authored-by: Dag Andersen <dagbjerreandersen@gmail.com>
@SIMULATAN
Copy link
Contributor Author

Ah, so that was why the CI failed. This issue reminded me once again of how complicated rust can be, foolish of me to think yolo-ing the change without a test compile would be a good idea 😅 thank you!
I think I may have to rebase + autosquash as GitHub can't do it during the merge, lmk when it's time to do so.

@dag-andersen
Copy link
Owner

Ah, so that was why the CI failed. This issue reminded me once again of how complicated rust can be, foolish of me to think yolo-ing the change without a test compile would be a good idea 😅 thank you! I think I may have to rebase + autosquash as GitHub can't do it during the merge, lmk when it's time to do so.

Yes, Rust is a struggle sometimes 😄 It looks fine as it is — I’ll merge it, and it will be part of the next release ✌🏻
Thank you for the contribution! I didn’t consider that URLs could potentially include uppercase letters.

@dag-andersen dag-andersen merged commit 19ce39f into dag-andersen:main Nov 7, 2024
2 checks passed
@dag-andersen
Copy link
Owner

v0.0.24 is now released 🚀 This fix is live

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