er-patcher icon indicating copy to clipboard operation
er-patcher copied to clipboard

Add "-l" for "--disable-rune-loss", fix and improve readme and er-patcher

Open Kazevic opened this issue 1 year ago • 2 comments

Kazevic avatar Sep 16 '22 00:09 Kazevic

Hi, thanks for this MR. I like the idea of using -l for the rune loss option but other than that I do have some issues with the MR:

  • The changes are mostly cosmetic and don't really solve any issue.
  • I get that streamlining the mish-mash of "disable" and "remove" is a cosmetic improvement but IMO that doesn't justify the introduction of breaking changes.
  • Currently, the options are sorted by inclusion in --all, why should this be changed?

gurrgur avatar Sep 18 '22 14:09 gurrgur

  • The "fix" part was referring to the typos; I wanted to put a title as short as possible. Also, even if they are mostly cosmetic, I think that it could save some time to people in practice, not to mention that they lower the file sizes.
  • What exactly became broken after the changes? I could try to fix it.
  • I sorted them by alphabetical order, but the order of inclusion was changed too, so it should not cause issues.

Kazevic avatar Sep 18 '22 21:09 Kazevic