Conversation
|
This pr seems to be fully ai-generated. @nh13 did you check all the new methods actually work ? If so, can you please include a short human written description of how you tested it ? |
|
@lovasoa I also started reviewing this before I noticed that it's LLM-output. It seems rather disrespectful to waste our time just because @nh13 is too lazy to put in their own time. Perhaps we should just close it as slop? My take on LLM contributions is that people are free to use whichever tools they want to improve their code, but this sort of qualifies as spam. |
|
@KnorpelSenf I also do find the lack of a single human word sad, but I think these new methods could be useful. The current string-based configuration is brittle. @nh13 : Nils, would you be ready to spend a little bit more time on this, switch this PR to "draft" status, and ask for our review again after you can show it all actually works ? If you are interested in contributing to this project, your help is welcome ! Like you, I also find LLMs useful, but I think we should all be careful when using them: they still often output nonsensical content, and I think it's unfair to ask others to review their outputs before you. |
|
All fair comments. Obviously I didn't get the right balance wanting to contribute back something I found useful locally in a rush, and that's unfair to you and the maintainers. Perhaps I could open an issue to ask for guidance before making any PR, would that help in the future. Thank-you for the feedback, I have taken it to heart. |
nh13
force-pushed
the
nh_typed-option-setters
branch
3 times, most recently
from
f16eb01 to
c8e1961
Compare
nh13
changed the title
Expose a `set_threads` convenience method on `Model` that takes a
`NonZeroU32`, following the existing `make_quiet()` pattern.
You can already do this with `set_option("threads", 4)`, but the typed
method makes it discoverable via IDE autocomplete and catches mistakes
at compile time (eg. wrong type, zero value).
nh13
force-pushed
the
nh_typed-option-setters
branch
from
c8e1961 to
a849b40
Compare
|
Apologies for the slop, that was my mistake. I've updated the PR to be just |
3 participants
(updated human description, sorry!)
Expose a
set_threadsconvenience method onModelthat takes aNonZeroU32, following the existingmake_quiet()pattern.You can already do this with
set_option("threads", 4), but the typed method makes it discoverable via IDE autocomplete and catches mistakes at compile time (eg. wrong type, zero value).