serenity icon indicating copy to clipboard operation
serenity copied to clipboard

man: Remove DeprecatedString

Open CarwynNelson opened this issue 1 year ago • 2 comments

Related to #17128

This pull request requires the minimum set of patches that I could think of to remove DeprecatedString from the man utility.

This includes:

  • Adding String variants to add_option and add_positional_argument in the ArgsParser
  • Converting render_to_terminal from DeprecatedString to ErrorOr<String> (which meant also changing the md utility)
  • Removing all of the DeprecatedString from man

When converting render_to_terminal to ErrorOr<String> I made a conscious decision to ignore the other methods on the class, even if they would have been simple to convert, in order to keep the patch as small as possible.

As discussed on the Discord another possible improvement here would be to have a version of Core::System::posix_spawnp that can take StringViews instead of const char*, but I think that is something best dealt with in another pull request.

CarwynNelson avatar Jun 13 '23 15:06 CarwynNelson

@LucasChollet Thanks for the review :)

I have pushed changes based on your comments

CarwynNelson avatar Jun 13 '23 16:06 CarwynNelson

Side note: Looks like we need a new variant of Core::System::posix_spawnp, because copying a StringView just to add a NUL byte is a bit silly. Currently it's necessary, yes, so let's merge this. (After the String-ArgsParser thing is done.)

Completely agree. This was something that was also discussed on the Discord

CarwynNelson avatar Jun 17 '23 20:06 CarwynNelson