Add valid chars test for the property of metadata #2601
Conversation
adzo261
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2601 +/- ##
==========================================
+ Coverage 95.15% 95.18% +0.02%
==========================================
Files 5 5
Lines 165 166 +1
==========================================
+ Hits 157 158 +1
Misses 8 8
Continue to review full report at Codecov.
|
lib/metadata.js
Outdated
| const properties = Array.isArray(metadata.property) ? metadata.property : [metadata.property]; | ||
| properties.forEach(function (property) { | ||
| if (property.match(/[A-Z]/)) { |
There was a problem hiding this comment.
🤔 I was wondering why you used a forEach to analyze the properties. Each file is already checked separately thanks to line 11. I think that we would only need current lines 90 to 92 to do the task in this case (+ in that way we have a much cleaner output error)
There was a problem hiding this comment.
Btw, just as we are here I've seen in the other file that in the test you included _, maybe we could throw spaces and _ to the Regex (as we really don't use them either). Take into account that we would need to change the error message to something like Property includes invalid characters
There was a problem hiding this comment.
But for the rest, really good work Aditya!! 💪. Thank you very much :)
There was a problem hiding this comment.
Thank you for reviewing and commenting.
I found that metadata.property was an array of strings in somecases.
And in other cases it was a normal string.
Hence, I had to add a check for array and run a loop over it.
And do we need to check symbols(non-alphanumeric) also? I can add that check too if we want it. And then we can change the error to more generic as you said.
There was a problem hiding this comment.
I found that
metadata.propertywas an array of strings ...
Ah okay, I didn't think about that. Very clever! In that case that part is perfect.
And do we need to check symbols(non-alphanumeric) also?
It would be nice to test for symbols too (except the dash - as we use kebab-case for properties). If you include dots take care, I don't think so, but maybe subproperties (like in the clipboard test) give you errors.
Thank you for reviewing and commenting.
Thanks to you for working on the PR :)
There was a problem hiding this comment.
From what I could understand, metadata.property can have only lowercase alphanumeric characters and a special symbol - for kebab-case.
Can you once confirm the set of symbols we can have?
And also does - "Property can have only lowercase alphanumeric characters and kebab symbol" makes sense as an error message then?
Post confirmation, I will start working on it.
There was a problem hiding this comment.
Yep, if I'm not mistaken only lowercase letters and dashes are allowed.
About the error message, maybe is more clear if it says dashes instead of kebab symbols.
There was a problem hiding this comment.
I have updated the PR. We can have a second round of review.
adzo261
changed the title
|
Looks good. Nice work @adzo261 |
Refers to issue #2595
As per our discussion,
metadata.propertycan contain onlylowercase alphanumeric characters and dashes.PR adds a test for the same, which raises an error if
metadata.propertyvalues doesn't have valid characters.Please have a look. Thanks!