-
-
Notifications
You must be signed in to change notification settings - Fork 495
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
Check for empty tags #557
Check for empty tags #557
Conversation
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.
This looks good to me. I just made a small remark about using else if
in one place for performance reasons.
src/TemplateData.js
Outdated
if (typeof data.tags === "string"){ | ||
data.tags = data.tags ? [data.tags] : []; | ||
} | ||
if (data.tags === null){ |
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.
This should be an else if
since the conditions are mutually exclusive. When typeof data.tags === "string"
, the second condition is evaluated again even though it cannot be true.
@kleinfreund thanks for looking so quickly. I've refactored it a bit so both checks are included. It sort of looked like the ternary check was trying to do this anyway - so I'm using that as well now. |
Oh damn @edwardhorsford sorry to do this but I thought your original commit was actually much easier to read. Can you switch back to that and use an Otherwise this is a 👍 from me |
@zachleat Updated as requested. It probably wants tests, but I'm not familiar with adding those, sorry. I also don't know if there's other stuff that gets merged that should also check for empty. |
Thank you @edwardhorsford!! |
Just added a test for this in f014f56 if you’d like to have a look @edwardhorsford (it’s one line 😇) |
Fixes #556 where empty tags causes the merge to fail.