Added check to validate Windows path separator #265 #268
Conversation
joachimmetz
requested review from
Onager and
grrrrrrrrr
and removed request for
Onager
Codecov Report
@@ Coverage Diff @@
## master #268 +/- ##
=========================================
+ Coverage 91.24% 91.3% +0.06%
=========================================
Files 7 7
Lines 411 414 +3
=========================================
+ Hits 375 378 +3
Misses 36 36
Continue to review full report at Codecov.
|
tools/validator.py
Outdated
| if source.type_indicator in ( | ||
| definitions.TYPE_INDICATOR_FILE, definitions.TYPE_INDICATOR_PATH): | ||
| if source.supported_os == [definitions.SUPPORTED_OS_WINDOWS]: | ||
| if source.separator != '\\': |
There was a problem hiding this comment.
This check kind of implies that all artifacts that support Windows need to have the separator set to \. If this is the case, why have the separator user defined at all?
There was a problem hiding this comment.
why have the separator user defined at all?
There are various different types of path separators: https://en.wikipedia.org/wiki/Path_(computing) and artifacts are intended to self contain the necessary information in the definition. We could create a WINDOWS_PATH but IMHO that does not improve this.
One additional thing the validator could check if the path contains more \ than '/' for Windows. This change is a short term fix to ensure existing definition (for at least Windows) are explicit about their path segment separator.
There was a problem hiding this comment.
Right I get the intention behind this. What I'm saying is that it will be confusing that when an artifact actually defines a path separator that is not '', let's say that artifact wants to use '/' instead. In that case, the validation will complain with "Missing path separator...", which doesn't make sense.
There was a problem hiding this comment.
I'll add the additional check for the number of \ versus '/' for now, but I somewhat agree with you on one side but on another side also would like this validated.
Added check to validate Windows path separator #265