Migrate asset and authors helpers to es6 #10611
Conversation
|
|
||
| const autolink = !(_.isString(options.hash.autolink) && options.hash.autolink === 'false'), | ||
| separator = _.isString(options.hash.separator) ? options.hash.separator : ', ', |
There was a problem hiding this comment.
I'm not fully sure if we can get rid of this logic, but it seems like we should be able to 🤔
There was a problem hiding this comment.
🤔Thinking if losing the isString check in the destructuring default value here has any downsides. Seems it differs only when non-string value is passed to separator, which should be wrong usage so can be ignored as case.
There was a problem hiding this comment.
Yeah, the only possible case I can think of is if the theme author passes in a variable (e.g. number of posts) 😟
There was a problem hiding this comment.
@vikaspotluri123 Ah yeah, but passing number of posts in as a separator is still wrong usage in a way right? 😄I mean previously we would have just ignored it and set , as separator, so I think the changes are still ok ;) Wdyt?
| limit = options.hash.limit ? parseInt(options.hash.limit, 10) : undefined, | ||
| visibilityArr = models.Base.Model.parseVisibilityString(options.hash.visibility); | ||
| module.exports = function authors(options = {}) { | ||
| options.hash = options.hash || {}; |
There was a problem hiding this comment.
Didn't use nested destructuring here because the input can be considered untrusted
vikaspotluri123
force-pushed
the
es6-assets-a
branch
from
e7a25f1
to
c8dcaa2
Compare
|
@rishabhgrg Could you please review & merge next Monday? Thaaanks 🙂 |
Refs #9589