-
Notifications
You must be signed in to change notification settings - Fork 41
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
Started using new code style. #111
base: master
Are you sure you want to change the base?
Conversation
@martinheidegger I can see if I can do it. My current full time job wants me spending 5-10% of my time on OSS/community stuff. I'm not sure exactly what needs to happen so if you can help point me in the right direction I'll do my best 😃. |
@itzsaga we are using @martinheidegger I was trying to add a test, but the package is broken with these changes right now. Seems to be something with Also, we are introducing a breaking change exporting a class instead a function. |
return '[WorkshopperFileError: ' + id + ' @ ' + error.exerciseFile + ']' | ||
} | ||
return error | ||
function fileError (id, exerciseFile) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could be changed to arrow function too
const fileError = (id, exerciseFile) => {
var i18n = this.i18n | ||
var buffer = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these variables could be updated to const
getFile: getFsObject.bind(null, 'file') | ||
} | ||
exports.getDir = (file, base) => getFsObject('dir', file, base) | ||
exports.getFile = (file, base) => getFsObject('file', file, base) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to declare all exports at final, so is more easy to know what is exported.
But could use the object shorthand
module.exports = {
idFromName,
dirFromName,
...
}
@ccarruitero the recent dependency update made a breaking change of making workshopper-adventure only compatible with Node > 8.
The linting doesn't fail but a few other things might. So I am opening this PR in the hopes that someone finds time to add tests to it.
@workshopper/javascripting-leads Maybe you want to take a look at this?