Please keep a single PR related to only one change. Let us just focus on the custom config file path change here. No need to add the custom cache path or do a version bump. Also please remove your test/.tldrrc.

Thanks.

Also, let us name the flag as --config.

@agnivade Actually the PR address only the custom config file. The custom cache path was already featured but not documented. Ok for the version bump.

@agnivade ok for --config also.

Also test/.mytldrrc is needed for the functional test.

Right. Could you please remove all the cache file related refactoring that you did ? It becomes hard to review multiple things ? Let us just focus to config file related changes here.

The only refactoring related to custom cache file is in the README, I can strip this off but not sure if this will help you.

I think your struggle is more related with the fact that config.get() was invoked in the constant declaration and not at runtime (not a best practice by the way); so in the aim of making config file a parameter I'd to transform the former constants in variables and enclose their default assignment in a function.

For example...

Before:

const CACHE_FOLDER = path.join(config.get().cache, 'cache');

After:

let cacheFolder = null;

function getCacheFolder() {
  if (!cacheFolder) {
    cacheFolder = path.join(config.get().cache, 'cache');
  }
  return cacheFolder;
}

You can see this in cache.js and in search.js (mmhh ... duplicate code by the way) and is not related with the custom cache path but with the custom config file path.

Hi @agnivade, sorry for pushing a little... any chance to see this landed anytime soon?

You could go and revert your changes of package.json while you wait 😉
Bumping dependency versions is ok but that's for another PR.

Hey @pliski, I will take some time to review this.

@vladimyr or you could fetch the repo and realize that is already done ;-D
check this commit 118aeaf

@vladimyr or you could fetch the repo and realize that is already done ;-D
check this commit 118aeaf

Now let me quote myself:

Bumping dependency versions is ok but that's for another PR.

And there is a lesson for you:
dependency versions !== package version

diff --git a/package.json b/package.json
index e4271cb..c5823fe 100644
--- a/package.json
+++ b/package.json
@@ -47,25 +47,25 @@
     "test:all": "npm run lint && npm test && npm run test:functional"
   },
   "dependencies": {
-    "chalk": "^2.3.0",
-    "commander": "^2.9.0",
-    "fs-extra": "^4.0.2",
-    "glob": "^7.1.2",
-    "lodash": "^4.17.4",
-    "marked": "^0.3.9",
+    "chalk": "^2.4.2",
+    "commander": "^2.20.0",
+    "fs-extra": "^8.0.1",
+    "glob": "^7.1.4",
+    "lodash": "^4.17.11",
+    "marked": "^0.6.2",
     "ms": "^2.0.0",
-    "natural": "^0.6.0",
-    "node-unzip-2": "^0.2.7",
-    "ora": "^2.0.0",
-    "os-homedir": "^1.0.1",
-    "request": "^2.75.0"
+    "natural": "^0.6.3",
+    "node-unzip-2": "^0.2.8",
+    "ora": "^3.4.0",
+    "os-homedir": "^2.0.0",
+    "request": "^2.88.0"
   },
   "devDependencies": {
     "env-test": "^1.0.0",
-    "eslint": "^4.6.1",
-    "husky": "^0.14.3",
+    "eslint": "^5.16.0",
+    "husky": "^2.3.0",
     "mocha": "^4.0.1",
-    "should": "^13.0.1",
-    "sinon": "^4.1.2"
+    "should": "^13.2.3",
+    "sinon": "^7.3.2"
   }
-}
+}
\ No newline at end of file

Also No newline at end of file is a bad thing, you know...

@vladimyr

And there is a lesson for you:

Are you being sarcastic?
If yes ... please stop.

If no, it's ok, I'm always keen on learning something new and improving myself.

So let's talk about code now 😄 .

@vladimyr

dependency versions !== package version

I misreaded your comment, sorry about that.
I'm reverting the dependency upgrades, even if it is really needed as some packages are nearly two years old.
So I'll make a separate PR if @agnivade is ok.

@vladimyr

Also No newline at end of file is a bad thing, you know...

I agree with you but not everyone does, in other projects I'm on the inverse is required.

To constraint this practice you can propose to enforce eol-last rule in .eslintrc.

so in the aim of making config file a parameter I'd to transform the former constants in variables and enclose their default assignment in a function.

Yes, this is the refactoring that I am talking about. Could you revert this, and just keep the --config flag changes in this PR ?

You can see this in cache.js and in search.js (mmhh ... duplicate code by the way) and is not related with the custom cache path but with the custom config file path.

Yes, this is not good. Also you are declaring a module-level variable, but only using it inside the function. Let's do all this later in a separate PR.

Yes, this is the refactoring that I am talking about. Could you revert this, and just keep the --config flag changes in this PR ?

unfortunately this is part of the "--config flag changes", the other changes wont work without.

This is what I meant when I said:

The only refactoring related to custom cache file is in the README,..

Ok, maybe I am missing something. I will look at it in details.

Sorry for the delay. It's on my list. I will get to it eventually.

I understand the issue now. Looking for a way to tackle this. Thanks for your patience @pliski

Hi @pliski - I found a simple way to work around the problem by just resetting the config variable once the file path is set. I created a new PR for this - #262.

You can see the change here - https://github.com/tldr-pages/tldr-node-client/pull/262/files#diff-f7bf2b665273dfa66ffa4ad7c0a52bb9R28

I agree that there are still lot of global variables in the code which pollutes the global state and makes code hard to modify. But just for this change, I think this is reasonable enough. We can refactor the code in a separate PR which can be reviewed properly.

If you are fine with this approach, then you can update your PR accordingly and we can get to reviewing and merging it. Thanks.

@pliski - Do you have any thoughts on this ?

Closing due to inactivity.