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.

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

Fix index.js generation for culture neutral codes #248

Merged
merged 1 commit into from Mar 29, 2017

Conversation

@gwynjudd
Copy link
Contributor

@gwynjudd gwynjudd commented Mar 27, 2017

E.g., for culture code 'nn', it will put

exports.nn = require('./nn.js')j;

into the index.js. Without this change, it fails at build time due to a lint rule

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Mar 27, 2017

can you point me the lint rule?

because to me you're fix looks a bit strange as both forms are equivalent.
Maybe we should fix the lint rule instead?

This was referenced Mar 27, 2017
@gwynjudd
Copy link
Contributor Author

@gwynjudd gwynjudd commented Mar 27, 2017

@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented Mar 27, 2017

I would disable the lint rule 😄

@gwynjudd gwynjudd force-pushed the gwynjudd:index branch from 885be03 to 0c6b37a Mar 27, 2017
@gwynjudd
Copy link
Contributor Author

@gwynjudd gwynjudd commented Mar 27, 2017

@BenjaminVanRyseghem I've disabled the rule

@@ -152,7 +152,8 @@ module.exports = function(grunt) {
return 'exports[\'' + file.replace('.js', '') + '\'] = require(\'./' + file + '\');';
}).join('\n');

fs.writeFileSync(dir + '/index.js', langFiles);
fs.writeFileSync(dir + '/index.js', '/* jshint sub: true */' + require('os').EOL);

This comment has been minimized.

@BenjaminVanRyseghem

BenjaminVanRyseghem Mar 28, 2017
Owner

can you use \n instead of require('os').EOL to be consistent with the rest of the file?

Without this rule, it will fail at build time if the languages include any
language neutral culture codes e.g. "es"
@gwynjudd gwynjudd force-pushed the gwynjudd:index branch from 0c6b37a to c638f66 Mar 28, 2017
@gwynjudd
Copy link
Contributor Author

@gwynjudd gwynjudd commented Mar 28, 2017

@BenjaminVanRyseghem BenjaminVanRyseghem merged commit 99e9e0f into BenjaminVanRyseghem:develop Mar 29, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gwynjudd gwynjudd deleted the gwynjudd:index branch Mar 29, 2017
@BenjaminVanRyseghem
Copy link
Owner

@BenjaminVanRyseghem BenjaminVanRyseghem commented May 2, 2017

in numbro 1.11.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.