Move file-specific options under OutFile

[andrews05]

This PR is addressing #220. It's not super important but it's a breaking change, so if it's something we want to do then I thought I should get it in now before the next release.

• pretend can become another variant of OutFile, probably OutFile::None, as that's what it essentially is - just another output destination and not a separate option
• backup and preserve_attrs should become properties of OutFile::Path variant (so that it would contain Path { path, backup, preserve_attrs }) as they don't have any effect on any other output and so semantically belong there best

Closes #220

[andrews05]

I'm having second thoughts about this. While it makes sense for the options to be part of OutFile, it does make it more cumbersome to use with no real benefit. I'll convert to a draft and see if anyone else wants to weigh in.

[AlexTMjugador]

I think this refactor makes sense in the context of applications that use OxiPNG as an intermediate optimization engine, which is a use case that the new raw API supports quite well. Asking those applications to pass options about whether to back up files when there are no files to begin with is semantically odd.

How do you think users will be affected by this refactor? If it only slightly complicates internal OxiPNG code, I think the cleaner client code it allows in some cases is worth it.

[andrews05]

My concern was that while the Options can just be filled with defaults (so you don't have to explicitly set everything), the associated values on an enum variant necessarily have to be provided. So if you're using the oxipng file API, you're now required to set those explicitly whereas before you weren't.

This isn't an issue for the Pretend option, just the backup and preserve_attrs. We could always go ahead with changing Pretend and leave the others for now, if we wanted.

[AlexTMjugador]

Right, I hear your concern. How about moving the inner enum variant fields to their own structs, as this StackOverflow question suggests? If you find that approach too unergonomic, perhaps adding constructor functions like OutFile::new_with_path could help.

[andrews05]

I actually did add OutFile::from_path when I realised how cumbersome it was to use in tests 😆

I just had another thought though: What if we deprecate/remove the backup option entirely? I feel it's a little archaic and unlikely that people actually use it. It's really a separate issue, but if it is something we want to do then it would affect what happens in this PR.

[AlexTMjugador]

I'd support removing the backup option, as I think backups are handled by other programs better than OxiPNG could ever do, but it'd be nice to gather some user feedback about that. I have the feeling that users of Unix-like OSes would approve that, while Windows users are more used to the "every program must do every feature any reasonable user could want from it" way of defining scope.

[andrews05]

Alright, I've reverted the backup flag so it remains in the Options struct. That makes OutFile::Path a little simpler and I do feel better about it now. We can discuss removing backup separately.
I'll mark this as ready for review again.

[andrews05]

@shssoichiro This PR and #546 are the last things on my to-do list for the next release. Once these are either merged or closed (depending on how you feel about them), I think we can do another release.

[AlexTMjugador]

I think these changes are good overall, but I'd like to address the potential removal of the backup option, which directly affects them.

My reading of the current state of the RFC at #542 is that there is no strong user opinion for or against removing backup, with the slightly more popular opinion being that it's okay to remove backup. There may be some use cases where it is useful, and the only technical concern with it at the moment is that it makes the OutFile::Path anonymous struct more complex.

However, this struct already contains two fields, and refusing to add more because of complexity is not a precedent that scales well. A third backup field should make no ergonomic difference, and thus it should not be a reason to remove backup. I think the from_path convenience function takes care of this well enough.

I do still think that backup is mostly unnecessary these days, but all of this makes me feel it would be better to either not remove it, or to deprecate it for removal at the next release after the upcoming one. With a deprecation notice, we could hopefully get more user feedback about it, and give any folks out there who depend on it time to move on.

Since the backup code is otherwise simple and unlikely to be a source of future maintenance effort, unlike deprecating an option for removal, I think it's best for everyone if we move backup to this struct and leave it alone.

[andrews05]

Thanks for the review @AlexTMjugador! You make some good points.

I think these changes are good overall, but I'd like to address the potential removal of the backup option, which directly affects them.

My reading of the current state of the RFC at #542 is that there is no strong user opinion for or against removing backup, with the slightly more popular opinion being that it's okay to remove backup. There may be some use cases where it is useful, and the only technical concern with it at the moment is that it makes the OutFile::Path anonymous struct more complex.

However, this struct already contains two fields, and refusing to add more because of complexity is not a precedent that scales well. A third backup field should make no ergonomic difference, and thus it should not be a reason to remove backup. I think the from_path convenience function takes care of this well enough.

This is true. The ergonomics wasn't so much a motivating factor for removing backup though, it was just what prompted the thought. If we do want to keep it, it could certainly go in here.

I guess regarding scaling, enum options with associated values inherently don't scale well. Adding a new option is necessarily a breaking change, whereas with the main options it generally isn't. If we want this to be scalable we should probably go the embedded struct route you suggested earlier, but at this point I think that may be overkill.

I do still think that backup is mostly unnecessary these days, but all of this makes me feel it would be better to either not remove it, or to deprecate it for removal at the next release after the upcoming one. With a deprecation notice, we could hopefully get more user feedback about it, and give any folks out there who depend on it time to move on.

Since the backup code is otherwise simple and unlikely to be a source of future maintenance effort, unlike deprecating an option for removal, I think it's best for everyone if we move backup to this struct and leave it alone.

Yeah, deprecation could be a good option. Though I think if we do deprecate it, with expectation to remove in a future version, it would seem awkward to move it in the interim. I'm not sure it's reasonable to mark associated values in an enum variant as deprecated, since you're still forced to provide them (at least if you're not using from_path()).

I'm not hugely opinionated on any of this one way another though. We could just close both PRs and keep the status quo if it's easier than figuring out what's best 😆

[AlexTMjugador]

This is true. The ergonomics wasn't so much a motivating factor for removing backup though, it was just what prompted the thought. If we do want to keep it, it could certainly go in here.

I guess regarding scaling, enum options with associated values inherently don't scale well. Adding a new option is necessarily a breaking change, whereas with the main options it generally isn't. If we want this to be scalable we should probably go the embedded struct route you suggested earlier, but at this point I think that may be overkill.

I thought a bit more about the whole idea of removing the backup option, and those are some good points, too! I think you're right: if this struct gets bigger, we should embed a named struct instead, but for now I think it's fine as is.

I'd be bold and go ahead with moving backup to this new struct to merge this PR. I don't expect this anonymous struct to grow much over time (what more options could we realistically add there?), and even if it does, we'll probably have plenty of reasons to do a semver-breaking release anyway.

[andrews05]

Ah, so you don't want to deprecate it? I still think it would be good to get rid of it someday, even if not today. We could still mark it deprecated in the CLI regardless of what we do in the API, to gather more feedback like you said (no API user in their right mind would be using it 😂).

[AlexTMjugador]

Hmm, I certainly didn't entertain the possibility of separating the API from the available command-line flags. From an API perspective, I'd just remove the flag (it's barely useful there as you said), while from a CLI perspective I'd be more conservative.

How easy do you think it'd be to make the API interface less coupled with the CLI switches? If it's elegant to do, I think the best way forward would be to deprecate it in the CLI, and remove it altogether from the API.

[andrews05]

Ooh, that's a good idea. I think that could be done for the backup switch, I'll give it a go.

[ace-dent]

My 2¢ as a CLI user:
Most scripts (once working) will run a tool quietly -q. I'm much more likely to notice when something breaks outright.
I'd still suggest making a breaking change and just remove the function. Whether you do it now, or next version, there will always be some pain for that 1% user...

[andrews05]

Yeah, that's also a good point. And even without -q there's a good chance they won't notice.
@AlexTMjugador What if we replaced it with an explicit failure message saying it's been removed and suggest using --out, --dir, or otherwise copying the files yourself?

I think those are our two best options right now. To sum up:

1. Remove it from the lib/API and make it part of the bin/CLI only, with deprecation warning.
2. Remove it completely, but with a nice message if you try to use it.

Either way it wouldn't be in the API. Which means this PR is probably good to go as-is, since changes to backup would be unrelated.

[andrews05]

Another reason to remove backup: It currently crashes if the input file has no extension 😂

[AlexTMjugador]

What if we replaced it with an explicit failure message saying it's been removed and suggest using --out, --dir, or otherwise copying the files yourself?

I like that idea, especially given that this feature also has the somewhat glaring bug you have just described, and fixing it would likely prompt a debate on how to handle files without extensions 😂

Which means this PR is probably good to go as-is, since changes to backup would be unrelated.

Yeah, I think so too. I'll go ahead and merge this!