-
Notifications
You must be signed in to change notification settings - Fork 170
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
chore: Use strum
to simplify GGUFArchitecture
maintenance
#334
chore: Use strum
to simplify GGUFArchitecture
maintenance
#334
Conversation
- Minimizes some noise when assigning `arch` from metadata (read via `candle` crate). - Custom `FromStr` can be replaced by `strum` derive, making it DRY, adding a new variant no longer needs to update the string mapping. - Both concerns now handled in smaller `from_value()` method.
Code Metrics Report=============================================================================== Language Files Lines Code Comments Blanks =============================================================================== Dockerfile 1 34 25 0 9 Happy 1 442 369 0 73 JSON 5 9 9 0 0 Python 21 741 622 21 98 TOML 15 389 352 1 36 ------------------------------------------------------------------------------- Jupyter Notebooks 1 0 0 0 0 |- Markdown 1 60 30 22 8 |- Python 1 96 87 1 8 (Total) 156 117 23 16 ------------------------------------------------------------------------------- Markdown 15 1026 0 758 268 |- BASH 6 205 192 0 13 |- Python 6 121 110 0 11 |- Rust 3 185 172 9 4 (Total) 1537 474 767 296 ------------------------------------------------------------------------------- Rust 84 27830 25514 353 1963 |- Markdown 40 419 0 407 12 (Total) 28249 25514 760 1975 =============================================================================== Total 144 30471 26891 1133 2447 =============================================================================== |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this, it makes the code much more idiomatic now. I think there are some Clippy lint failures, though. After those are fixed, I will be happy to merge.
Changes kept the `Result` of `.to_string()` which was not compatible with the type returned via `.and_then()` call from `strum`. `.context()` is added to work like `.expect()` with contexual error, but without unwrapping the result so that `.and_then()` can be used with it.
I assume you have to approve the workflow run? I have no errors with clippy or rustfmt on my end. If it complains about |
All green, can we merge this? I'll then continue with #335 BTW, I noticed you're using merge commits? Do you prefer that I clean up the commit history? I tend to do a PR in logical commits, especially larger ones. Then during PR I address any other concerns but I don't tend to alter the prior commit history for the PR. I think it works well, but isn't the cleanest approach if you don't use Squash as the PR merge strategy. I personally prefer squash so that the main branch has a clean commit history, no interleaving commits from parallel PR work. A squash commit still includes a reference to the PR on github where you have the commit history and discussion for context if ever needed. I find it also helps with Something to consider 😎 |
Absolutely! If you could do that it would be great. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Just updated the history into a single commit but was too slow 😂 Thanks! I'll get the other PR sorted now 👍 |
Perhaps you could open a separate PR for this? Looking forward to the other PR! |
Sorry?
You may want to consider squash merge for future PRs though. You just select it via the drop-down instead of your current default merge strategy: |
Yes, I do plan on doing that in the future, though. |
Minor improvement to consider?
arch
from metadata (that is read via thecandle
crate).FromStr
can be replaced bystrum
derive, making it DRY, adding a new variant no longer needs to update the string mapping.from_value()
method.