Add hjson support #30

ghost · 2017-09-28T11:31:41Z

Adding HJSON support, does not support json override
Started in haraka/Haraka#2124

P.S.: All test work but someone should really check what I wrote, but I assume you do it anyways

codecov · 2017-09-28T11:33:33Z

Codecov Report

Merging #30 into master will increase coverage by 0.54%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #30      +/-   ##
==========================================
+ Coverage   80.73%   81.27%   +0.54%     
==========================================
  Files           7        8       +1     
  Lines         410      422      +12     
  Branches      118      123       +5     
==========================================
+ Hits          331      343      +12     
  Misses         79       79

Impacted Files	Coverage Δ
readers/hjson.js	`100% <100%> (ø)`
config.js	`89.74% <100%> (+0.7%)`	⬆️
configfile.js	`70.24% <100%> (+0.14%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce7243a...a584727. Read the comment docs.

ghost · 2017-09-28T18:21:20Z

The Codecov is now messed up as I accidentally pushed to the same branch. The accident is removed but the Codecov stayed.
Sorry...

msimerson · 2017-09-28T18:34:40Z

config.js

@@ -54,7 +54,7 @@ Config.prototype.getDir = function (name, opts, done) {
 };

 function merge_config (defaults, overrides, type) {
-    if (type === 'ini' || type === 'json' || type === 'yaml') {
+    if (type === 'ini' || type === 'hjson' || type === 'json' || type === 'yaml') {


after a few conditionals, it gets cleaner to convert this to a switch/case. Agree?

Yes, will do

msimerson · 2017-09-28T18:52:44Z

configfile.js

+            }
+
+            name = yaml_name;
+            type = 'yaml';
        }


This entire section doesn't seem right. The gist of this section is to let a .yaml file override a missing json file. You want to extend it to let a yaml override a hjson or json file. The easiest way to fix it is to revert this section back and then make this alteration:

--- a/configfile.js +++ b/configfile.js @@ -338,11 +338,11 @@ cfreader.load_config = function (name, type, options) { if (!fs.existsSync(name)) { - if (!/\.json$/.test(name)) { + if (!/\.h?json$/.test(name)) { return cfrType.empty(options, type); } - const yaml_name = name.replace(/\.json$/, '.yaml'); + const yaml_name = name.replace(/\.h?json$/, '.yaml'); if (!fs.existsSync(yaml_name)) { return cfrType.empty(options, type); }

I made it so HJSON can be overridden by JSON and also YAML, but I might be wrong

Hrmm, I'm wondering how useful that is. Using YAML in place of JSON? Easy to see the use case. Using HJSON instead of JSON, against easy to see. Using JSON in place of HJSON? Why not just specify JSON? What's the point? (If there is a good point, it should be added in the docs.)

Ok, I get it. So, should I change it to allow both to be overridden by YAML? Am I right?

Correct. Basically treat hjson the same as json.

msimerson · 2017-09-28T18:54:42Z

test/config.js

+const hjsonRes = {
+    matt: 'waz here',
+    array: [ 'has an element' ],
+    objecty: { 'has a property': 'with a value' }


The contents of this test file should be different than the contents of the json file, so that we're assuring that the correct file got read and parsed.

Ok, understood

msimerson · 2017-09-28T21:29:04Z

Whoohoo, looks great except that you've also added a package-lock.json, which is likely something that npm just did "for you." Maybe we should add that to .npmignore for now?

ghost · 2017-09-28T21:50:20Z

Yes, definitely it should be in .gitignore or something.

msimerson · 2017-09-28T21:51:58Z

Do feel free to add it. I think it's NPM 5+ that drops that file by default, so we'll likely be adding that to .gitignore in all the repos in haraka/*.

ghost · 2017-09-28T21:54:03Z

Ok, should I include it in this PR or should I create a new one?

msimerson · 2017-09-28T21:54:43Z

Either way is fine.

msimerson · 2017-09-28T21:56:36Z

readers/hjson.js

+const Hjson = require('hjson');
+
+exports.load = function (name) {
+    return Hjson.parse(fs.readFileSync(name, "utf8"));


Oh, just noticed this: per the JS conventions, the case for the variable names of libraries is always lower case, except for constructors.

I was going to use it first, but then I went with what hjson's npm description gives as example. Shall be fixed.

See: https://stackoverflow.com/questions/1564398/javascript-method-naming-lowercase-vs-uppercase (we deviate from the conventions slightly with the use of under_score names instead of camelCase, but we're pretty consistent otherwise.)

Ok, I will remember that.

msimerson · 2017-09-28T22:01:58Z

fixes haraka/Haraka#2124

msimerson · 2017-09-28T22:04:35Z

thanks @PSSGCSim

baudehlo · 2017-09-28T23:30:09Z

https://stackoverflow.com/questions/44206782/do-i-commit-the-package-lock-json-file-created-by-npm-5 This link seems to suggest we should keep the package-lock file.

…

On Sep 28, 2017, at 6:04 PM, Matt Simerson ***@***.***> wrote: thanks @PSSGCSim — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

ghost · 2017-09-29T00:50:28Z

If you read it, I do not think this file is all that needed. Because npm creates it itself while running npm install and it only stores integrities and such regarding the installation (the sub-modules). If we would need to do so I think package.json should be sufficient. I think this would be important only if you would like to host configuration of an instance on git. Like for updating and fetching exactly same modules at each deployment.
Correct me if I am wrong.

baudehlo · 2017-09-29T00:57:57Z

I’m no fan of npm 5. It caused me tons of headache and lost time at work. I just wanted to show what the general consensus was.

…

On Sep 28, 2017, at 8:50 PM, Jan Schwarzrock ***@***.***> wrote: If you read it, I do not think this file is all that needed. Because npm creates it itself while running npm install and it only stores integrities and such regarding the installation (the sub-modules). If we would need to do so I think package.json should be sufficient. I think this would be important only if you would like to host configuration of an instance on git. Like for updating and fetching exactly same modules at each deployment. Correct me if I am wrong. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

ghost · 2017-09-29T01:02:09Z

I have linked the npm's docs onto this issue in the .gitignore. e82a402#diff-a084b794bc0759e7a6b77810e01874f2
I am not all that experienced with Node.js/npm but from what I got it is not needed.
Again, please correct me if I am wrong.

baudehlo · 2017-09-29T02:26:49Z

Totally not needed. It’s a question of does it benefit or not. I don’t have an opinion either way.

…

On Sep 28, 2017, at 9:02 PM, Jan Schwarzrock ***@***.***> wrote: I have linked the npm's docs onto this issue in the .gitignore. e82a402#diff-a084b794bc0759e7a6b77810e01874f2 I am not all that experienced with Node.js/npm but from what I got it is not needed. Again, please correct me if I am wrong. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

msimerson · 2017-09-29T17:50:10Z

While acknowledging that the consensus appears to be: check it in, my opinion is that if we do decide to, we should wait until after we drop support for node 6 (and with it, npm 3). For now, I'm personally avoiding npm 5 for a while longer, thanks to show-stopper bugs. Shucks, NPM 5 is what finally convinced me to give yarn a try. For now, I'm sticking with npm 3 for a while longer, while npm 5 gets most of the pathological bugs ironed out.

baudehlo · 2017-09-29T18:02:25Z

I agree with you Matt. I've had the same problems. npm5 was a clusterfuck for us.

…

On Fri, Sep 29, 2017 at 1:50 PM, Matt Simerson ***@***.***> wrote: While acknowledging that the consensus appears to be: check it in, my opinion is that if we do decide to, we should wait until after we drop support for node 6 (and with it, npm 3). For now, I'm personally avoiding npm 5 for a while longer, thanks to show-stopper bugs. Shucks, NPM 5 is what finally convinced me to give yarn a try. For now, I'm sticking with npm 3 for a while longer, while npm 5 gets most of the pathological bugs ironed out. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#30 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAobY8kbuRUeoM6MhT463pJOa0AF9C4Vks5snS3TgaJpZM4PnH6B> .

add hjson support

1ab0d5b

ghost mentioned this pull request Sep 28, 2017

hjson configuration haraka/Haraka#2124

Closed

KingNoosh requested review from msimerson and a team and removed request for msimerson September 28, 2017 13:02

msimerson reviewed Sep 28, 2017

View reviewed changes

refactor long if statement to switch

4f66a38

msimerson reviewed Sep 28, 2017

View reviewed changes

PSSGCSim added 2 commits September 28, 2017 21:22

remove unnecessary fallback from HJSON > JSON > YAML

916ec35

fix-up HJSON tests

eabe034

rm unwanted file

3183946

msimerson self-requested a review September 28, 2017 21:52

msimerson approved these changes Sep 28, 2017

View reviewed changes

msimerson reviewed Sep 28, 2017

View reviewed changes

PSSGCSim added 2 commits September 28, 2017 23:58

add package-lock.json to .gitignore

b375f87

fix hjson library name to be lowercase

a584727

msimerson merged commit e82a402 into haraka:master Sep 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hjson support #30

Add hjson support #30

ghost commented Sep 28, 2017

codecov bot commented Sep 28, 2017 •

edited

Loading

ghost commented Sep 28, 2017

msimerson Sep 28, 2017

ghost Sep 28, 2017

msimerson Sep 28, 2017 •

edited

Loading

ghost Sep 28, 2017 •

edited by ghost

Loading

msimerson Sep 28, 2017

ghost Sep 28, 2017

msimerson Sep 28, 2017

msimerson Sep 28, 2017

ghost Sep 28, 2017

msimerson commented Sep 28, 2017

ghost commented Sep 28, 2017

msimerson commented Sep 28, 2017

ghost commented Sep 28, 2017

msimerson commented Sep 28, 2017

msimerson Sep 28, 2017 •

edited

Loading

ghost Sep 28, 2017

msimerson Sep 28, 2017

ghost Sep 28, 2017

msimerson commented Sep 28, 2017

msimerson commented Sep 28, 2017

baudehlo commented Sep 28, 2017 via email

ghost commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

ghost commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

msimerson commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

Add hjson support #30

Add hjson support #30

Conversation

ghost commented Sep 28, 2017

codecov bot commented Sep 28, 2017 • edited Loading

Codecov Report

ghost commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

ghost Sep 28, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson commented Sep 28, 2017

ghost commented Sep 28, 2017

msimerson commented Sep 28, 2017

ghost commented Sep 28, 2017

msimerson commented Sep 28, 2017

msimerson Sep 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msimerson commented Sep 28, 2017

msimerson commented Sep 28, 2017

baudehlo commented Sep 28, 2017 via email

ghost commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

ghost commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

msimerson commented Sep 29, 2017

baudehlo commented Sep 29, 2017 via email

codecov bot commented Sep 28, 2017 •

edited

Loading

msimerson Sep 28, 2017 •

edited

Loading

ghost Sep 28, 2017 •

edited by ghost

Loading

msimerson Sep 28, 2017 •

edited

Loading