'no such file' error on first save at LOCAL_FILE_PATH #41
Conversation
constants.js
Outdated
|
|
||
| exports.LOCAL_SAVE_DIR = localSaveDir; | ||
| exports.LOCAL_FILE_PATH = localFilePath; | ||
| exports.PUSHED_FILE_PATH = pushedFilePath; |
There was a problem hiding this comment.
Although these changes are fine, I don't see the need for them. Can you please explain your rationale? Since the path is basically a constant for a particular OS, I don't see any reason to introduce the three new function calls. Thanks! 🙂
There was a problem hiding this comment.
Your LOCAL_SAVE_DIR idea is required! Thanks for that, but I would just define it as a constant instead of using path.join.
There was a problem hiding this comment.
Hi,
This is a best practice in my opinion since it ensures that the path is not malformed or at least normalized. Using path should handle all use cases but on the other hand it boils down to preference.
I will honor your opinion on this and change it accordingly.
There was a problem hiding this comment.
Hmm, I think xdg-basedir is dependable. 🙂 I know what they say about micro-optimizations, but it's still a few function calls I would avoid.
(If things go bad and a bug comes up in the future, because of this, then I will be sure to own up to my error in judgment 😸 )
file-module.js
Outdated
| var filepath = require('./constants').LOCAL_FILE_PATH; | ||
|
|
||
| if (!fileExists(filepath)) { | ||
| if(!fileExists.sync(filepath)) { |
There was a problem hiding this comment.
Drop the file-exists dependency and use fs.existsSync instead.
file-module.js
Outdated
| @@ -49,7 +49,7 @@ exports.writeToPushed = function (glob) { | |||
| var xdg = require('xdg-basedir'); | |||
| var filepath = require('./constants').PUSHED_FILE_PATH; | |||
|
|
|||
| if (!fileExists(filepath)) { | |||
| if(!fileExists(filepath)) { | |||
There was a problem hiding this comment.
Use fs.existsSync here. Drop file-exists.
file-module.js
Outdated
| @@ -49,7 +49,7 @@ exports.writeToPushed = function (glob) { | |||
| var xdg = require('xdg-basedir'); | |||
| var filepath = require('./constants').PUSHED_FILE_PATH; | |||
|
|
|||
| if (!fileExists(filepath)) { | |||
| if(!fileExists(filepath)) { | |||
| var mkdirp = require('mkdirp'); | |||
| mkdirp(xdg.data + '/cube'); | |||
package.json
Outdated
| "pretty-ms": "^1.2.0", | ||
| "read": "^1.0.6", | ||
| "pretty-ms": "^3.0.1", | ||
| "read": "^1.0.7", |
There was a problem hiding this comment.
Whoa whoa. This is too much of a change! Bumping major versions is a gamble, and I don't want to do it without thorough testing. So, let's keep this out of the scope of this particular PR. As I mentioned in the other issue's comment, I will freeze versions after merging this PR, test the whole module and if everything works, make a release.
Deprecated warnings suck, but not as much as a module that stops working!
|
Hey, thanks for all your changes. I want to refocus this PR on just creating a new constant in the The unit test you added is fine and required. Once I merge the template, I will add a few more tests. |
icyflame#41 review changes
icyflame#41 review changes
|
@icyflame I just pushed the changes we have discussed. I hope you find everything in order now. |
|
Indeed, everything is in order now. I have merged it into master. I am going to let it be there till maybe tomorrow before freezing everything. Doing a thorough test and then publishing a new version to NPM! Thanks a lot for the time you spent working on this 😄 |
|
Glad I was able to help, I enjoyed it very much and can hopefully provide more help in the future. |
This request resolves #38, possible even #33.
This change requires the package.json to be updated to the latest versions just to be save.