poise icon indicating copy to clipboard operation
poise copied to clipboard

Get rid of autoref-specialisation

Open GnomedDev opened this issue 1 year ago • 4 comments

This significantly simplifies poise internals, by removing the fancy autoderef specialization used to fallback from the poise traits, to the serenity traits, and finally to FromStr.

It is replaced by normal implementations, which does not significantly impact usability as users may define newtypes to implement the traits themselves if needed.

This PR is currently missing:

  • testing to make sure I didn't break anything

GnomedDev avatar Nov 01 '24 01:11 GnomedDev

What sort of runtime testing is required for this?

mkrasnitski avatar May 19 '25 22:05 mkrasnitski

What sort of runtime testing is required for this?

I'm guessing just testing if all the current things that parse still parse the same way, check the existing impls and cross reference i guess?

If that all works it should be fine

jamesbt365 avatar May 20 '25 00:05 jamesbt365

I'm not even sure what the specialization was trying to solve. Was it a workaround for trying to write a blanket implementation of impl<T: FromStr> SlashArgument for T in order to avoid overlapping impls (and likewise for PopArgument)?

mkrasnitski avatar May 20 '25 01:05 mkrasnitski

Yes.

GnomedDev avatar May 20 '25 15:05 GnomedDev

Tested this out a bit and it looks great other than the review comment, haven't had any other issues.

jamesbt365 avatar Jul 04 '25 12:07 jamesbt365

Tested this out a bit and it looks great other than the review comment, haven't had any other issues.

Can you test to verify the behavior change around string arguments, where whitespace is now trimmed?

mkrasnitski avatar Jul 08 '25 15:07 mkrasnitski

Superseded by https://github.com/serenity-rs/poise/pull/357

arqunis avatar Jul 14 '25 19:07 arqunis