LatLng: stricter arguments validation #7765
Conversation
johnd0e
force-pushed
the
source-data-validation
branch
from
63a1ea4
to
82a14ac
Compare
There was a problem hiding this comment.
I like this PR 👍
Maybe you want to generalize this PR and add checks for L.Point to #7432
| } | ||
| } | ||
|
|
||
| function checkNumber(a) { |
There was a problem hiding this comment.
I would suggest to move this into Utils, maybe we will have some other places where we want to use this. (#7432)
There was a problem hiding this comment.
I do not agree, because the function implementation is heavily defined by it's particular application.
E.g. we may need integer numbers, or allow Infinity, etc.
There was a problem hiding this comment.
Hmm you are right but I still think it would be cleaner to have on general function that can used outside of Leaflet core too. Maybe a second optional parameter allowedTypes can be passed as array ['integer', 'double', 'Infinity'].
For infinity it would be also possible to use if(value === Infinity || checkNumber(value))
There was a problem hiding this comment.
Maybe a second optional parameter
allowedTypescan be passed as array
Too many parameters to take into account all possible cases. E.g. should it accept hex strings? Exponential form?
BTW, the function itself is not perfect too. I'd like to improve it, but in reasonable limits. May be you have ideas.
There was a problem hiding this comment.
But we can define / document which cases / types are allowed.
A way to get 0x10 managed is to check if the string is starting with 0, length > 1 and the second char is not . for decimal numbers. With that 0010 is invalid too.
There was a problem hiding this comment.
Too much actions. Can be performance penalties
In separate PR. |
johnd0e
force-pushed
the
source-data-validation
branch
2 times, most recently
from
246353e
to
f582a8e
Compare
spec/suites/geo/LatLngSpec.js
Outdated
| var c = new L.LatLng("010", "0x10"); // better'd throw here.. | ||
| expect(c.lat).to.be(10); | ||
| expect(c.lng).to.be(0); // bad! |
There was a problem hiding this comment.
Here are the cases that I do not like
| // do not throw atm: | ||
| // expect(L.latLng).withArgs('0010', 0).to.throwError(); | ||
| // expect(L.latLng).withArgs('0x10', 0).to.throwError(); | ||
| // expect(L.latLng).withArgs('1e5', 0).to.throwError(); |
There was a problem hiding this comment.
1e5 = 100000 is a correct number, what is not vaild in my eyes is that the string '1e5' is converted to 1 (parseInt('1e5'))
There was a problem hiding this comment.
is a correct number
Sure, but do we really expect such number in Leaflet?
There was a problem hiding this comment.
Maybe in CRSSimple but I don't think so. I think we should only check if it is a valid number and not if the number makes sense
johnd0e
force-pushed
the
source-data-validation
branch
4 times, most recently
from
39de142
to
137bbc3
Compare
johnd0e
force-pushed
the
source-data-validation
branch
from
137bbc3
to
e5543dc
Compare
Throw on any invalid value of lat/lng/alt (alt is validated if is explicitly specified)
johnd0e
force-pushed
the
source-data-validation
branch
from
e5543dc
to
d0cd9b0
Compare
|
Found on stackoverflow: https://run.plnkr.co/plunks/93FPpacuIcXqqKMecLdk/ |
|
Infinity is now not valid anymore. And I would say if someone uses |
Throw on any invalid value of lat/lng/alt
(alt is validated if is explicitly specified)
Ref: #7128
Question:
undefined/nullin arguments - should we throw instead of returningundefined/null?alt, or better allowundefinedvalue for compatibility purposes?