feat: custom config file path #255

pliski · 2019-05-18T12:43:09Z

Description

Address #254.
Allow to specify a custom config file path instead of the default one (~/.tldrrc).
This will allow to use different repositories, themes, cache directories with the same tldr instance.

Example

~/.tldrrc

{
    "pagesRepository": "https://github.com/tldr-pages/tldr",
    "repository": "https://tldr-pages.github.io/assets/tldr.zip",
    "cache": "/home/user/tldr/default",
    "themes": {
    // ...
    },
    "theme": "base16"
}

~/.mytldrrc

{
    "pagesRepository": "https://github.com/myname/myrepo",
    "repository": "https://mypages.github.io/assets/tldr.zip",
    "cache": "/home/user/tldr/default",
    "themes": {
    // ...
    },
    "theme": "ocean"
}

Aliases

alias mytldr='tldr --config-file ".mytldrrc"'

Usage

tldr tar

mytldr mate

Checklist

Please review this checklist before submitting a pull request.

Code compiles correctly
Created tests, if possible
All tests passing (npm run test:all)
Extended the README / documentation, if necessary

allow to define a different config-file name (default ~/.tldrrc)

agnivade · 2019-05-18T13:53:21Z

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.

agnivade · 2019-05-18T13:56:17Z

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

pliski · 2019-05-19T01:51:37Z

@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.

pliski · 2019-05-19T01:52:18Z

@agnivade ok for --config also.

pliski · 2019-05-19T01:55:48Z

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

agnivade · 2019-05-19T07:20:01Z

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.

This reverts commit b1e9117.

pliski · 2019-05-19T10:15:06Z

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.

pliski · 2019-05-21T11:47:12Z

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

vladimyr · 2019-05-21T12:37:44Z

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

agnivade · 2019-05-21T16:52:39Z

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

pliski · 2019-05-21T19:03:15Z

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

vladimyr · 2019-05-21T23:19:52Z

@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...

pliski · 2019-05-22T08:47:47Z

@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 😄 .

pliski · 2019-05-22T08:51:50Z

@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.

pliski · 2019-05-22T09:00:01Z

@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.

agnivade · 2019-05-23T08:38:54Z

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.

pliski · 2019-05-24T15:11:44Z

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,..

agnivade · 2019-05-24T16:50:27Z

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

agnivade · 2019-05-30T16:42:07Z

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

agnivade · 2019-06-12T07:46:09Z

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

agnivade · 2019-06-15T14:10:00Z

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.

agnivade · 2019-11-18T03:21:35Z

@pliski - Do you have any thoughts on this ?

agnivade · 2020-04-21T17:25:17Z

Closing due to inactivity.

pliski added 5 commits May 18, 2019 12:00

feat: custom config-file path

d69e24a

allow to define a different config-file name (default ~/.tldrrc)

fix: module.exports on cache.js

18b3837

doc: --config-file option in README, and some refactor

38db60f

test: add functional test for --config-file option

e2574ec

version bump

b1e9117

Revert "version bump"

118aeaf

This reverts commit b1e9117.

pliski closed this May 19, 2019

pliski reopened this May 19, 2019

rafactor: option --config-file to --config

9a10822

revert: dependency version upgrade

143d781

This was referenced May 22, 2019

Deps upgrade #257

Closed

deps: dependecies upgrade #258

Merged

Merge remote-tracking branch 'upstream/master'

5c5ebb6

agnivade closed this Apr 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: custom config file path #255

feat: custom config file path #255

pliski commented May 18, 2019

agnivade commented May 18, 2019

agnivade commented May 18, 2019

pliski commented May 19, 2019

pliski commented May 19, 2019

pliski commented May 19, 2019

agnivade commented May 19, 2019

pliski commented May 19, 2019

pliski commented May 21, 2019

vladimyr commented May 21, 2019

agnivade commented May 21, 2019

pliski commented May 21, 2019

vladimyr commented May 21, 2019

pliski commented May 22, 2019

pliski commented May 22, 2019 •

edited

Loading

pliski commented May 22, 2019

agnivade commented May 23, 2019

pliski commented May 24, 2019 •

edited

Loading

agnivade commented May 24, 2019

agnivade commented May 30, 2019

agnivade commented Jun 12, 2019

agnivade commented Jun 15, 2019

agnivade commented Nov 18, 2019

agnivade commented Apr 21, 2020

feat: custom config file path #255

feat: custom config file path #255

Conversation

pliski commented May 18, 2019

Description

Example

Checklist

agnivade commented May 18, 2019

agnivade commented May 18, 2019

pliski commented May 19, 2019

pliski commented May 19, 2019

pliski commented May 19, 2019

agnivade commented May 19, 2019

pliski commented May 19, 2019

pliski commented May 21, 2019

vladimyr commented May 21, 2019

agnivade commented May 21, 2019

pliski commented May 21, 2019

vladimyr commented May 21, 2019

pliski commented May 22, 2019

pliski commented May 22, 2019 • edited Loading

pliski commented May 22, 2019

agnivade commented May 23, 2019

pliski commented May 24, 2019 • edited Loading

agnivade commented May 24, 2019

agnivade commented May 30, 2019

agnivade commented Jun 12, 2019

agnivade commented Jun 15, 2019

agnivade commented Nov 18, 2019

agnivade commented Apr 21, 2020

pliski commented May 22, 2019 •

edited

Loading

pliski commented May 24, 2019 •

edited

Loading