Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extensions with period #27

Open
asherber opened this issue Dec 4, 2019 · 5 comments
Open

Extensions with period #27

asherber opened this issue Dec 4, 2019 · 5 comments

Comments

@asherber
Copy link

asherber commented Dec 4, 2019

Thanks for this useful library!

I know this would be a breaking change, but I'd like to suggest that the values in FileFormat.Extension should start with a period, just like the output of Path.GetExtension() -- and like the table in this repo's README.

@neilharvey
Copy link
Owner

I'm in two minds about this one. It probably does make sense to align with the way that Path.GetExtension works, but is that a good enough reason to create a breaking change?

I'm tempted to leave this issue open for a little while to give a chance to hear some more opinons on this.

@asherber
Copy link
Author

asherber commented Dec 6, 2019

I totally understand. Maybe this is something you just want to hang onto until the next major release, rather than making a major release just for this.

Two other possibilities would be a static property, something like FileFormatInspector.ExtensionHasDot, which instructs file formats to include a dot before the extension (defaults to false) or an additional optional extensionHasDot parameter to the FileFormatInspector constructor which accomplishes the same thing.

I'm happy to work on a PR if you like one of those approaches.

@tiesont
Copy link

tiesont commented Dec 6, 2019

Totally useless comment: I'm ambivalent, either way. As long as I know how it works in any given version, I'm fine with the change. Might be nice to match Path.GetExtension, if for no other reason than maybe it has a history and therefore has set the precedent?

@neilharvey
Copy link
Owner

@tiesont It's useful to hear that it wouldn't break what your use case, so thanks for the feedback :)

I've thought about this a bit over the weekend, and I'm inclined to change it. Having consistency with the standard libraries would help to make the usage more predicable, so that's probably a good enough reason to change it.

I also had a look at some of the other System.IO APIs, FileInfo.Extension works the same way, and Path.ChangeExtension produces the same regardless of whether the input which helped to convince me.

@asherber Thanks for the suggestion, but to make that work I'd have to make the formats mutable (either at the static or instance level) to make the option work which doesn't really appeal - I prefer having the formats fixed as immutable types (because a format doesn't change). I think what I'd prefer to do is just accept the breaking change and include the period in the definitions of each format.

@kevgeni
Copy link
Contributor

kevgeni commented Jul 27, 2023

If I may leave an opinion, at this point it's been 2 years, and this library has quite some download in nuget, so i'll err on the side of don't break existing app.
This is my example code that will break if it changes. Currently I remove the . from .net to compare it with this library. Is there a way to make it full proof? yes. Is that useful, or is everyone going to make it fullproof when first using it ? I doubt. Modifying the behaviour to add a period to FileSignature might break some code and it's not worth it 2 years later.

string extension = Path.GetExtension(file.FileName).TrimStart('.');
if (_allowedImageType.Select(q => q.Extension).Distinct().Contains(extension, StringComparer.InvariantCultureIgnoreCase) == false)
{
    throw new BadHttpRequestException("Bad file extension");
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants