haraka/haraka-config

use path.extname instead of regex comparisons

msimerson opened this issue · 16 comments

msimerson commented

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

Deleted user commented

Are we talking about this if block?

haraka-config/configfile.js

Lines 339 to 353 in e82a402

if (!fs.existsSync(name)) {
if (!/\.h?json$/.test(name)) {
return cfrType.empty(options, type);
}
const yaml_name = name.replace(/\.h?json$/, '.yaml');
if (!fs.existsSync(yaml_name)) {
return cfrType.empty(options, type);
}
name = yaml_name;
type = 'yaml';
cfrType = cfreader.get_filetype_reader(type);
}

msimerson commented

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

msimerson commented

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

Deleted user commented

Oh, ok I will fix it.

Deleted user commented

If this is the purpose should I do something about this line too?

haraka-config/config.js

Line 120 in e82a402

if (/^(ini|value|list|data|h?json|yaml|binary)$/.test(args[i])) {

msimerson commented

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.

Deleted user commented

Ok from this:

haraka-config/config.js

Lines 131 to 137 in e82a402

if (!fs_type) {
if (/\.hjson$/.test(fs_name)) fs_type = 'hjson';
else if (/\.json$/.test(fs_name)) fs_type = 'json';
else if (/\.yaml$/.test(fs_name)) fs_type = 'yaml';
else if (/\.ini$/.test(fs_name)) fs_type = 'ini';
else fs_type = 'value';
}

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

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
Deleted user commented

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

Perl habits die hard, eh?

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

msimerson commented

where single quote usually means single character

Interesting, in what context?

Deleted user commented

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

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. ;-)

Deleted user commented

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

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