Consider reworking arguments to file()

[sschuberth]

I'm always having trouble remembering what checks exactly are performed for the different parameters to file(). For example, does exists = false check whether the file does not exist and fail otherwise? (No, it does not.) While that's part of the documentation, I believe this could be made more clear by renaming parameters. How about:

• exists -> verifyExists
• fileOkay -> canBeFile
• folderOkay -> canBeDir
• writable -> mustBeWritable
• readable -> mustBeReadable

[ajalt]

I agree that that parameter names can be improved, and I like your suggestions. Changing parameter names is a breaking change, and, unlike with functions, there's no way to deprecate a parameter.

So in this case, I'd prefer backwards compatibility over clarity.

[sschuberth]

Changing parameter names is a breaking change, and, unlike with functions, there's no way to deprecate a parameter.

True. I had briefly thought about how to remedy that, and my idea was to simply double all parameters during some transition period. That is, the function signature would be

fun RawOption.file(
        // Old parameters.
        exists: Boolean = false,
        fileOkay: Boolean = true,
        folderOkay: Boolean = true,
        writable: Boolean = false,
        readable: Boolean = false,
        // New parameters.
        verifyExists: Boolean = false,
        canBeFile: Boolean = true,
        canBeDir: Boolean = true,
        mustBeWritable: Boolean = false,
        mustBeReadable: Boolean = false,
)

When using named parameters you could choose to only user the new names. However, that's still not exactly nice by itself, and you would have to do some internal alignment of old and new parameters.

Another option would be to simply introduce a new function, like .path(), that uses the new parameter names. But I agree that's also just a work-around.

So in the end I'm completely fine with you closing this issue as-is, or maybe keeping it around until it's time for a breaking change, like for a new major version of clikt.

[ajalt]

Thinking about it more, we could maintain backward compatibility and use @Deprecated if the function signature changed i.e. we add a new parameter. If anyone has input on a new check the functions could do, I could add it and deprecate the old names at the same time.

[sschuberth]

That's a neat trick. How about a new parameter canBeSymlink that I was actually thinking about before?

[ajalt]

Yeah, that's perfect.

[sschuberth]

FYI, I recently discovered a neat trick to identify symlinks that also works on Windows.

[ajalt]

Thanks! Neither that method or Files.isSymlink seem to work on symlinks created by cywgin, and I'm not aware of any native windows symlink. How would one create a symlink on windows in a way that method could detect?

[sschuberth]

There are actually two kinds of native "symlinks" on Windows: Junctions (for directories only; since Windows 2000) and "real" symbolic links (since Windows Vista). Both of these should be detected by my code snippet. The best tool I know to create / handle these on Windows is the Link Shell Extension.

[sschuberth]

Cygwin, on the other hand, by default uses its own home-brewn file format to simulate symlinks. That feature dates back to when Windows did not have any kind of native symlinks, i.e. it's ancient. I do not think it's worth the effort to support these at all, esp. as nowadays Cygwin can be told to create native symlinks instead.

[ajalt]

So in the end it seems that this trick doesn't quite work. It works perfectly for calls like file(writable=true), but aparantly Kotlin resolves to the method with the fewest parameters in the face of ambiguity. So if we add a new overload with an optional parameter, a call like file() or file(true) will always resolve to the old function, and will always be marked as deprecated, even after applying the ReplaceWith quick fix.

[sschuberth]

Hmm, thanks for giving this a try anyway. So either we simply discard the whole issue, or we accept that something like file(true) does not work nicely, which maybe is not as bad as it sounds, as it's good practice to use named parameters for functions taking (multiple) booleans anyway.

[ajalt]

I decided on a solution that isn't perfect, but that I'm okay with:

I added the canBeSymlink parameter to a new overload and deprecated the existing version as discussed. In addition, I added another overload with no parameters that isn't deprecated. This way calls with no arguments (e.g. file()) aren't marked as deprecated.

Calls with unnamed arguments (e.g. file(true, false)) will still always be marked as deprecated, but I doubt that pattern is common. People can always remove the deprecation by adding names to the arguments.

[sschuberth]

Thanks for this!