OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Rename `GetFilenamesFromDirectory` #324

Closed DanRStevens closed 4 years ago

DanRStevens commented 4 years ago

I was thinking, GetFilenamesFromDirectory is kind of long, and not very information dense. What if we just renamed it to something like Dir. Much like the DOS command (and a common alias on Linux), Dir simply returns the contents of a directory. This would also match up with other languages, such as Ruby, where you can get directory listings with syntax such as Dir['*.vol'].


If we wish to restrict results to files (or rather not-directories), we might also introduce a DirFiles.

Alternatively, we could expose the template method to allow for arbitrary filtering of results. Of course, it may be nice to provide a few convenience helper wrappers so end users don't have to use lambdas if they wish to avoid that. There's also the concern of namespace pollution, in that if the template is moved to the header file, the header would also need to import the filesystem namespace (or rather the fs alias).

Brett208 commented 4 years ago

I think the names Dir and DirFiles are okay. It wouldn't bother me to see Directory and DirectoryFiles instead of the abbreviations although Dir as an abbreviation is probably standard enough to be recognized by most.

If we wanted to emulate Dir[*.vol] function, we could just use regex. This might actually remove a need for an extension specific overload. In this case it might be nice to add a comment with a template indicating how to pull all files with a specific extension.

I think maybe encouraging the use of regex template *.vol$ might be appropriate? I havn't tested it.

-Brett

DanRStevens commented 4 years ago

I think I like having the file extension overload. Regex syntax can be more verbose and hard to understand. Most of the time, people are probably just filtering by extension, and that can be done more simply and more cheaply without resorting to using regex. Regex is good to have though, since it's far more general in what it can do.

Or is this more about shell globs versus an extension overload. The example given uses shell glob syntax. I suppose that would be more natural than passing just an extension. It's also simpler than using full regex syntax.

Extension: ".vol" Glob: "*.vol" Regex: ".*\.vol"

Another point is the .extension() method doesn't consider names that begin with . to be an extension, such as .git/.

Edit: Maybe we should use the name DirFilesWithExtension, so we can leave room for a shell glob overload named DirFiles. Both extensions and globs would be std::string parameters, so they can't both be overloads of the same method name.


Slightly off topic side point, but Ruby has a shell glob extension, where ** can match any number of directory levels. Hence **/*.txt would recursively match all .txt files in all subfolders. Very handy. I'm not looking to implement it at this time, just plant the seed in case we ever need to extend for such a use case.


I wouldn't mind spelling things out in full and avoiding the Dir abbreviation. One caveat though is that Directory is a noun, with no corresponding verb usage. The abbreviation Dir is often used in a verb context (being the name of a command), and so feels a bit more natural there.

DanRStevens commented 4 years ago

Hmm, do we even use the regex version anywhere? Nothing comes up in my source code searches. I'm wondering if we should perhaps just remove it. I know it's increased functionality, though if we're not using it, and have no plans to use it, maybe it shouldn't be there? It would be easy enough to add back if we ever needed it.

The other side to that argument is this is library code, so it sometimes pays to be slightly more general than you need. You never know what the requirements of the users of the library will be.

Brett208 commented 4 years ago

Just wanted to mention that I originally added the regex version in the mind to be generic for library code as mentioned. There is one place in OP2Archive where I've thought about using Regex for the filename search. I'll try to point it out when integrating this new version of OP2Utility.