Fix #746 - Expand FileComparison to Rename/Patch cases

[sloshy]

Fixes #746

Took a slightly different approach than the one suggested. Because it seems like there are two distinct cases, I felt the user of the library should not have to know that these fields are mutually exclusive, and it should be instead clear from the models. If the patch field exists, it should be a String and not Option[String] since it sounds like that means there will also be no previous_filename field (and vice versa).

I've added two additional cases for FileComparison as FileComparisonRename and FileComparisonPatch that contain these two exclusive cases as separate models. It involves a little copy/paste but nothing egregious and I think this is better for end-users than the alternative anyway.

Either way, this is a breaking model change and should be noted when released.

EDIT: based on the below discussion, it seems the fields are not mutually exclusive but in the case of a rename there will always be previous_filename but not always a patch. I've modified the cases to match, and I've added methods to get these from the common supertype if they exist as Option[String].

[sloshy]

In the event that these are not exclusive cases, I'm happy to make the change to them being Option[String] or maybe changing the cases around somewhat. Curious if there is a third case for "rename + patch" as well.

[sloshy]

Seems they are exclusive according to: https://docs.github.com/en/rest/reference/commits#compare-two-commits

For example, files with a renamed status have a previous_filename field showing the previous filename of the file, and files with a modified status have a patch field showing the changes made to the file.

In the examples below it looks like the status cannot be both modified and renamed and in the modified case it's not clear that previous_filename can still possibly appear.

[fedefernandez]

Also, would it make sense to add the fields in the trait (as a defs)? I think it could be helpful in handling the common fields

sealed trait FileComparison {
  def sha: String
  //...
}

[sloshy]

Done. Also added a couple helper methods for getPatch and getPreviousFilename for the cases where you don't necessarily want to do pattern matching.

[rojsrojs]

Seems they are exclusive according to: https://docs.github.com/en/rest/reference/commits#compare-two-commits

For example, files with a renamed status have a previous_filename field showing the previous filename of the file, and files with a modified status have a patch field showing the changes made to the file.

In the examples below it looks like the status cannot be both modified and renamed and in the modified case it's not clear that previous_filename can still possibly appear.

The status cannot be both modified and renamed. However, the change can have both the fields previous_filename and patch. If you make a small modification and a rename at the same time, the status will be rename and have the patch field with the small change and previous_filename populated. If you make a big change, we will get two file comparisons and one of them will be removed and the other one added and the previous_filename won't appear in any of them. I suspect you get the status modified if you make a "medium" change with the previous_filename populated, but I haven't tested that case.

[sloshy]

Thanks @rojsrojs for your inputs. I've changed the two cases as follows:

• Renamed to FileComparisonModified and FileComparisonRenamed
• For the Modified case, it has patch but no previous_filename
• For the Renamed case it has previous_filename and an optional patch field.
• Getters were added to the base trait so users don't have to pattern match to get these fields, but they return Option[String] without pattern matching so they are not guaranteed.

[sloshy]

Just noticed and fixed an encoder/decoder ambiguity. Because the cases are separated now and can be parsed as each other, I've modified the encoder/decoder to take into account the expected status of the Renamed case. I've also changed the Modified case to be NotRenamed as I felt that made more sense given the possibility for it to be an added or deleted file.

[fedefernandez]

Left a minor comment, but it looks good to me.

[fedefernandez]

Would it make sense to remove the getPatch and getPreviousFilename fields from here? If the user wants these fields, they must match the concrete type. If they don't need those fields, they can use the supertype.

[sloshy]

I think it's a lot less verbose to use these rather than do a proper pattern match for some cases. If I'm using this library and I want "the previous filename if it exists" then that's a single method call. Or if I want the patch "if it exists" that's also a single call. Takes up a lot less vertical space. The main use case of doing the pattern match would be if you're passing the file comparison to some downstream function and want to validate that it's the right subtype first, but I don't think it should be required if we can help it.