Cost Surface - shapefile converting & extracting PU cost/id #219
Conversation
|
This pull request is being automatically deployed with Vercel (learn more). marxan – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan/5mD6uVukov3SiWe8Mzcp1cbFJBe2 marxan-storybook – ./app🔍 Inspect: https://vercel.com/vizzuality1/marxan-storybook/FLzzbdY4BjmXcGqVKTsBgAPzA7ko |
kgajowy
force-pushed
the
geoprocessing/surface-cost
branch
from
28e1865
to
a2f77e1
Compare
|
kgajowy
force-pushed
the
geoprocessing/surface-cost
branch
from
a2f77e1
to
9975f73
Compare
kgajowy
force-pushed
the
geoprocessing/surface-cost
branch
from
9975f73
to
3904d4f
Compare
| }); | ||
|
|
||
| describe(`when given GeoJson has pu costs`, () => { | ||
| it(`throws exception`, () => { |
There was a problem hiding this comment.
the test description doesn't sound right
| describe(`when given GeoJson isn't a FeatureCollection`, () => { | ||
| it(`throws exception`, () => { | ||
| expect(() => sut.extract(fixtures.simpleGeometry())).toThrow( | ||
| /is supported/, |
There was a problem hiding this comment.
super minor - we'll see the test passing, but asserting that an exception will be raised containing the text is supported seems slightly misleading (why should an error be thrown if something "is supported"?): in this specific case, maybe matching case-insensitively the full text of the error (/Only FeatureCollection is supported/i) should make things clearer for whoever skims through these tests in the future.
| cost: number; | ||
| planningUnitId: string; | ||
| }; | ||
| type MaybeProperties = Partial<Properties> | undefined | null; |
There was a problem hiding this comment.
this is quite useful in general - I suggest to create a MaybeSome<T> and then type MaybeProperties = MaybeSome<Properties> or something like that.
There was a problem hiding this comment.
| }; | ||
| type MaybeProperties = Partial<Properties> | undefined | null; | ||
|
|
||
| export class ExtractPuCost implements PuExtractorPort { |
There was a problem hiding this comment.
I am not too keen on class names based on verbs - why don't we name this PuCostExtractor?
There was a problem hiding this comment.
I think this would also match some of your previous code
Includes moving
isDefinedinto lib (util)(yes, I messed up branch name with switching surface/cost order 😅 )