-
Notifications
You must be signed in to change notification settings - Fork 219
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
Directory parameter extension - RegEx #114
base: master
Are you sure you want to change the base?
Conversation
lib/ImageData.js
Outdated
if ( typeof directory === "object" && directory.regex && directory.regex.find ) { | ||
const regex = new RegExp(directory.regex.find); | ||
const output = this.dirName.replace(regex, directory.regex.replace || ""); | ||
return path.join(output, prefix + fileName + suffix + extension); |
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 RegExp
is bad idea a bit.
Because:
- Now configuration is
json
format. so we have to write regexp as string. - Configuration seems to become complicated.
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.
Sure, I agree with complexity part, at least partially. It's because I think that better flexibility always will introduce more complexity, in one or different form. Also, keep in mind that we are not replacing old simple approach, we are just adding extension to it. So the RegEx
configuration could be used only by more advanced users. But I guess I must agree, that this could increase number of incoming questions and bugs :)
If you feel that the path-template approach is simpler, we can start from there.
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.
Take a look at: https://github.com/ysugimoto/aws-lambda-image/pull/116/files
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.
OK, now we can use path-template
approach.
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.
yeah, I plan to do that
# Conflicts: # lib/ImageData.js # package.json # test/image-data.js
Please don't merge it yet, It's not fully finished :) |
ava
dependency to0.18.2
configtest
(in progress)