This was meant to be a small change, but it spiralled into a big one.
The original intention was to separate OptionsResult and OptionsError. With these types separated, the Help and Version variants can only be returned from the Options::parse function, and the later option-parsing functions can only return success or errors.
Also, Misfire was a silly name.
As a side-effect of Options::parse returning OptionsResult instead of Result<Options, Misfire>, we could no longer use unwrap() or unwrap_err() to get the contents out. This commit makes OptionsResult into a value type, and Options::parse a pure function. It feels like it should be one, having its return value entirely dependent on its arguments, but it also loaded locales and time zones. These parts have been moved into lazy_static references, and the code still passes tests without much change.
OptionsResult isn't PartialEq yet, because the file colouring uses a Box internally.
I read through every file and applied a couple of rustfmt suggestions. The brace placement and alignment of items on similar lines has been made consistent, even if neither are rustfmt's default style (a file has been put in place to enforce this). Other changes are:
• Alphabetical imports and modules
• Comma placement at the end of match blocks
• Use newlines and indentation judiciously
• Spaces around associated types
• Spaces after negations (it makes it more clear imho)
• Comment formatting
• Use early-returns and Optional `?` where appropriate
These are holdovers from how I used to write Rust ("back in the day" of 2014). There are still some places in the code where I think it's worth glob-importing enums, but not these places.
This commit makes changes to the way variables are referenced:
• Make types Copy when possible
• Make methods take `self` instead of `&self` where possible (trivially_copy_pass_by_ref)
• Remove unnecessary borrowing (needless_ref)
• Remove unnecessary cloning (clone_on_copy)
• Remove `ref` from match arms where possible (new Rust match ergonomics)
This commit uses Clippy to fix all the 'use_self' warnings. Using Self instead of the type name has been good Rust style for a while now, and it's become the style I'm used to seeing.
Using --octal_permissions will insert another column before the existing
permissions where permissions are encoded using octal values as
requested in #316
- Checking on a directory doesn’t tell us if supported elsewhere
(some filesystems, like tmpfs, don’t support created time)
- We want to be able to display a column even if some subfiles or
subdirectories don’t support it
So now if unsupported a time of zero is used, and displayed as `-`
The Default impls for DefaultFormat and LoadFormat were originally called ‘new’, to which Clippy suggested that they be changed. But as these functions change based on what the year is, a function called something other than ‘new’, like ‘load’.
This commit changes the definition of Arg so that it knows about which values it can accept, and can display them in the help text. They were already being shown in the help text, but they were passed in separately, so one argument could show two different sets of options if it wanted. Now, the argument itself knows whether there are suggestions, so it doesn’t have to be passed in separately.
This means we can use it for other things, including listing choices when an option is missed out, without having to repeat the list.
With Misfire::BadArgument now only having two fields, it’s not worth using a constructor function anymore.
This commit meddles about with both the Colours and the FileExtensions.
Even though all the renderable fields were turned into traits, the FileName struct kept on accessing fields directly on the Colours value instead of calling methods on it. It also did the usual amount of colour misappropriation (such as ‘punctuation’ instead of specifying ‘normal_arrow’)
In preparation for when custom file colours are configurable (any day now), the colourise-file-by-kind functionality (links, sockets, or directories) was separated from the colourise-file-by-name functionality (images, videos, archives). The FileStyle struct already allowed for both to be separate; it was only changed so that a type other than FileExtensions could be used instead, as long as it implements the FileColours trait. (I feel like I should re-visit the naming of all these at some point in the future)
The decision to separate the two means that FileExtensions is the one assigning the colours, rather than going through the fields on a Colours value, which have all been removed. This is why a bunch of arbitrary Styles now exist in filetype.rs.
Because the decision on which colourise-file-by-name code to use (currently just the standard extensions, or nothing if we aren’t colourising) is now determined by the Colours type (instead of being derived), it’s possible to get it wrong. And wrong it was! There was a bug where file names were colourised even though the rest of the --long output wasn’t, and this wasn’t caught by the xtests. It is now.
Just because the type that gets used right now is Copy and Clone doesn’t mean that when we pass mock ones in for tests they’ll be those two as well. So we have to go through and add &s everywhere.
No new features here, just some restructuring. Mode::GridDetails was nice and elegant with those two fields, but now there’s a grid-details-only option the elegance has gone out the window.
Some of the deduce functions used to just blatantly call std::env::var_os and not care, introducing global state into a module that was otherwise nice and functional and self-contained. (Well, almost. There’s still terminal width.)
Anyway, this made it hard to test, because we couldn’t test it fully with this global dependency in place. It *is* possible to work around this by actually setting the environment variables in the tests, but this way is more self-documenting.
With this in place, we can start to unit test things like deriving the view by passing in what the $COLUMNS environment variable should be, and that’s one of the first things checked.
src/options/mod.rs *almost* has all its tests moved to where they should be!
Sometimes, the type in the Ok part of the Result wouldn’t implement PartialEq, so the first macro (which uses assert_eq) won’t work. In these cases, this new macro can be used instead, which just unwraps the Err’s contents. In other cases, it can shave off a ) at the end of a few lines.
The table Options struct is roughly half runtime configuration and half flags to select which columns to display The column fields might as well be in their own struct, and now that the ‘for_dir’ function doesn’t use SizeFormat, it can be moved to Columns.
This changes the SizeFormat option parser from its old, strict-by-default behaviour (where passing both --bytes and --binary would be an error) to the new, use-the-last-argument behaviour (where passing --bytes --binary would use --binary because it came later).
Doing this meant adding functionality to Matches so that it could return *which* argument matched. Previously, the order of --bytes and --binary didn’t matter, because they couldn’t both be present, but now it does.
The assert_parses function was problematic because it insisted on using assert_eq! to check its contents. This won’t work for any type we want to test that doesn’t implement PartialEq, such as TimeFormat, which holds references to years and date strings and other such.
To go about fixing this, the first step is to change that function so it only does the initial processing, rather than the assertion, which is now done outside of it in the test macros instead.
