Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

'no such file' error on first save at LOCAL_FILE_PATH #41

Merged
merged 8 commits into from
Oct 3, 2017
Merged

'no such file' error on first save at LOCAL_FILE_PATH #41

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0ff57fd
apply codestyle to file-module
Jan-Ka Oct 1, 2017
9a4310e
added file-module.js tests
Jan-Ka Oct 1, 2017
2bed021
add LOCAL_SAVE_DIR to constants and build other values from that
Jan-Ka Oct 1, 2017
5ae16d9
used synchronous functions for checkLocalFile
Jan-Ka Oct 1, 2017
8a31c06
get rid of (most) deprecation warnings by updating to latest packages
Jan-Ka Oct 1, 2017
5247235
removed path function calls from constants
Jan-Ka Oct 3, 2017
a447916
replaced 'file-exists' dependency with call to 'fs'
Jan-Ka Oct 3, 2017
64ef8bd
undo package.json upgrade
Jan-Ka Oct 3, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
var xdg = require('xdg-basedir');
exports.LOCAL_FILE_PATH = xdg.data + '/cube/times.csv';
exports.PUSHED_FILE_PATH = xdg.data + '/cube/pushed.csv';
var path = require('path');

var localSaveDir = path.join(xdg.data, '/cube');
var localFilePath = path.join(localSaveDir, 'times.csv');
var pushedFilePath = path.join(localSaveDir, 'pushed.csv');

exports.LOCAL_SAVE_DIR = localSaveDir;
exports.LOCAL_FILE_PATH = localFilePath;
exports.PUSHED_FILE_PATH = pushedFilePath;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! 🙂

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your LOCAL_SAVE_DIR idea is required! Thanks for that, but I would just define it as a constant instead of using path.join.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😸 )

8 changes: 4 additions & 4 deletions file-module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ exports.writeLocal = function (solvetime, scramble) {
exports.checkLocalFile = function () {
var fileExists = require('file-exists');
var touch = require('touch');
var xdg = require('xdg-basedir');
var filepath = require('./constants').LOCAL_FILE_PATH;

if (!fileExists(filepath)) {
if(!fileExists.sync(filepath)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the file-exists dependency and use fs.existsSync instead.

var mkdirp = require('mkdirp');
mkdirp(xdg.data + '/cube');
var localSaveDir = require('./constants').LOCAL_SAVE_DIR;
mkdirp.sync(localSaveDir);
touch(filepath);
}

Expand Down Expand Up @@ -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)) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use fs.existsSync here. Drop file-exists.

var mkdirp = require('mkdirp');
mkdirp(xdg.data + '/cube');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use mkdirp.sync here.

touch(filepath);
Expand Down
24 changes: 12 additions & 12 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,23 @@
"statistics"
],
"dependencies": {
"bar-horizontal": "^0.2.0",
"charm": "^1.0.0",
"cli-color": "^1.0.0",
"configstore": "^1.0.0",
"fast-csv": "^0.6.0",
"fast-stats": "0.0.2",
"file-exists": "^0.1.1",
"gh-gist-owner": "^1.0.1",
"jsonfile": "^2.0.1",
"bar-horizontal": "^0.3.0",
"charm": "^1.0.2",
"cli-color": "^1.2.0",
"configstore": "^3.1.1",
"fast-csv": "^2.4.1",
"fast-stats": "0.0.3",
"file-exists": "^5.0.0",
"gh-gist-owner": "^1.1.0",
"jsonfile": "^4.0.0",
"keypress": "^0.2.1",
"lodash.max": "^4.0.1",
"lodash.min": "^4.0.1",
"math-avg": "^1.0.0",
"meow": "^3.1.0",
"meow": "^3.7.0",
"mkdirp": "^0.5.1",
"pretty-ms": "^1.2.0",
"read": "^1.0.6",
"pretty-ms": "^3.0.1",
"read": "^1.0.7",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

"remove-min-max": "^1.0.0",
"request-json": "^0.5.3",
"scrambo": "^0.1.0",
Expand Down
13 changes: 13 additions & 0 deletions test/file-module.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
var assert = require('assert');
var fs = require('fs');
var constants = require('../constants');
var fileModule = require('../file-module');

it('creates LOCAL_SAVE_DIR', function () {
fileModule.checkLocalFile();

var expected = true;
var actual = fs.existsSync(constants.LOCAL_FILE_PATH);

assert.equal(actual, expected);
});