Subfolders to Dependencies #115
Conversation
There was a problem hiding this comment.
@ishitamed19
Thanks for a quick PR. The overall changes look good. I have a few suggestions.
-
Is there a way to avoid repeating the defaultPrefPath in all the modules? What if we use the global technique?
-
Please open a PR to dev branch from a feature branch. The recommended PR process is outlined here. The only modification to the given advice is not to squash your commits.
-
The file permissions are being changed from 644 to 755. Since all of the files are interpreted by Node.js interpreter and the launch point controlled by npm via package.json, there is no advantage to having an executable permission to any of the files. Please keep the default file permissions at 644.
-
You are using the complete file name for constructing the alias. Instead if you use only to directory level (say @input ---> cli/input), then we get two advantages:
- Less number of aliases
- Less number of directories in input/ and output/
- Fewer package.json files.
-
version numbers for all aliases is looking tedious and repetitive. Please remove them.
| @@ -1,5 +1,5 @@ | |||
| const program = require('caporal'); | |||
| const preferenceManager = require('../utils/preference-manager'); | |||
| const preferenceManager = require('@utils/preference-manager'); | |||
|
|
|||
There was a problem hiding this comment.
If we use the globals technique, then
global.__defaultPrefPath = path.join(__dirname, '../../../../default-prefs.json');
statement can be placed here and removed from every where else.
There was a problem hiding this comment.
This would be a good idea, sir. We can also read a json into the global scope.
| "caporal": "^0.9.0", | ||
| "chalk": "^2.3.0", | ||
| "cli-spinner": "^0.2.8", | ||
| "cli-table": "^0.3.1", | ||
| "controller": "file:lib/controller", |
There was a problem hiding this comment.
It is a single dependency for all the files inside lib/controller folder. Will rename it to @controller for better readability
There was a problem hiding this comment.
To add to what @ishitamed19 said. Whenever we import the folder as a module directly, it takes index.js file as the entry point. Therefore, there is no need to add such a dependency on package.json.
That said, it would be recommended to have an alias such as @controller as we import the contents of the modulle in the tests.
|
@mukkachaitanya Please review the PR. Also there is a build failure. It is good to resolve the failure in this PR itself. |
Yes Sir. I am testing it out. |
|
@ishitamed19 Please rebase your code to the latest commit of the dev branch. |
Requirements
Description of the Change
Benefits
Resolves issue #104
Possible Drawbacks
Multiple Views support