haraka / haraka-config

Haraka config file loader and parser
https://www.npmjs.com/package/haraka-config
MIT License
10 stars 13 forks source link

use path.extname instead of regex comparisons #31

Closed msimerson closed 6 years ago

msimerson commented 6 years ago

In configfile, we have an if/else chain that inspects a filename to determine its type. Instead, we should likely use path.extname.

ghost commented 6 years ago

Are we talking about this if block? https://github.com/haraka/haraka-config/blob/e82a40252b40fd2e70e08514f51d313560cb5a6b/configfile.js#L339-L353

msimerson commented 6 years ago

Bah I wrote the wrong filename. It's here in config.js

msimerson commented 6 years ago

The general ideas being, make that one less place to require code maintenance when we add support for some future file type.

ghost commented 6 years ago

Oh, ok I will fix it.

ghost commented 6 years ago

If this is the purpose should I do something about this line too? https://github.com/haraka/haraka-config/blob/e82a40252b40fd2e70e08514f51d313560cb5a6b/config.js#L120

msimerson commented 6 years ago

Hrmm, a string found there would be the user explicitly specifying the file content type. As you can see, some of the types (value, list, data) don't map to filename extensions. I don't know what you can do there that would improve it.

ghost commented 6 years ago

Ok from this: https://github.com/haraka/haraka-config/blob/e82a40252b40fd2e70e08514f51d313560cb5a6b/config.js#L131-L137 I boiled it down to this:

if (!fs_type) {
    var fs_ext = path.extname(fs_name);

    switch (fs_ext) {
        case ".hjson":
        case ".json":
        case ".yaml":
        case ".ini":
            fs_type = fs_ext.substring(1);
            break;

        default:
            fs_type = 'value';
            break;
    }
}

Opinion?

msimerson commented 6 years ago

Almost perfect. You'll fail lint tests because of the var. Use const instead. The other thing I'd do differently (not required) is consistently use single quotes. They're easier to read and I'm in the habit of always using ' unless I "want" interpolation.

baudehlo commented 6 years ago

Perl habits die hard, eh?

On Fri, Sep 29, 2017 at 3:39 PM, Matt Simerson notifications@github.com wrote:

Almost perfect. You'll fail lint tests because of the var. Use const instead. The other thing I'd do differently (not required) is consistently use single quotes. They're easier to read and I'm in the habit of always using ' unless I "want" interpolation.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/haraka/haraka-config/issues/31#issuecomment-333219804, or mute the thread https://github.com/notifications/unsubscribe-auth/AAobY3wO--vp6zNb7b3n-tE8VB6_zejCks5snUdogaJpZM4PnvHL .

ghost commented 6 years ago

Ok, got it. I tend to use double quotes because they mean string where single quote usually means single character. But I will abide.

msimerson commented 6 years ago

Perl habits die hard, eh?

LOL. Actually, sh/bash/POSIX, which predate my perl habits and which I still use very frequently.

msimerson commented 6 years ago

where single quote usually means single character

Interesting, in what context?

ghost commented 6 years ago

Mainly C# and like. I am not sure but I think C and C++ also do this.

Edit: I was correct, https://stackoverflow.com/a/3683613/1829884.

msimerson commented 6 years ago

Darn you! I've tried so hard to forget the C I nearly learned so long ago, and now you're running my nose in it. ;-)

ghost commented 6 years ago

I did one small change please take a look before I issue a PR. https://github.com/PSSGCSim/haraka-config/commit/bdaaa02a7e6db410151922d054251bd4c1304fa7 It is regarding where the substring (line 132) occurs.

msimerson commented 6 years ago

Even better, getting rid of the redundant leading dots. 👍