koopjs / FeatureServer

An open source Geoservices Implementation (deprecated)
https://geoservices.github.io
Other
104 stars 32 forks source link

Added server and layer info customization options. #173

Closed hparkertt closed 4 years ago

hparkertt commented 4 years ago

Added ability to override id, defaultVisibility, minScale, and maxScale in layer info via metadata object.

NOTE: removed hard-coded OBJECTID values in testing in favor of variables, to help account for hash function differences between windows/unix.

Resolves #170

rgwozdz commented 4 years ago

Hey @hparkertt - sorry for the delay, reviewing this ASAP.

hparkertt commented 4 years ago

@rgwozdz I'm going to use before functions, rather than beforeEach functions, just because that feels like the more correct solution. To me, it doesn't make sense to get the pertinent OBJECTIDs before every test. My thought is that we should get all of the OBJECTIDs prior to testing to ensure they are not changing between tests. Syntactically, the only difference is that I have to add the objectIds property on the this object, rather than on the this.currentTest object. I'm going to hold off on pushing another commit until I hear back from you on this. Here's the before function pattern I'm going to use:

// ./test/query.js
before(function () {
  const response = FeatureServer.query(data, { outFields: 'OBJECTID' })
  this.objectIds = response.features.map(feat => feat.attribtues.OBJECTID)
})

// ./test/route.js
before(function () {
  const response = FeatureServer.query(data, { outFields: 'OBJECTID' })
  this.secondOBJECTID = response.features[1].attributes.OBJECTID
})

Also, as an aside, I had tried this pattern before but couldn't get it to work. After doing a little more research this time around, I discovered that there is a scoping issue when using lambda functions with mocha. There's a note in their documentation about it here. It might make sense to create a separate 'housekeeping' ticket (not sure what label you would use) to convert lambda functions over in an attempt to stay consistent. I can see this as a 'gotcha' that could crop up from time to time if the convention is to use lambda functions in the tests. Thoughts?

rgwozdz commented 4 years ago

@hparkertt - Totally agree on the before instead of beforeEach, wrote my review comment in haste, good catch.

Yeah, makes sense that it won't work with arrow functions, as the this would be from the parent scope. At this point, we don't have an established convention, other than to use arrow functions unless there is a reason not to. This seems like a good reason. Thanks for checking in.

rgwozdz commented 4 years ago

Thanks @hparkertt. We'll release this shortly.

rgwozdz commented 4 years ago

Apologies for release delay. Now in 2.22.0. Thanks again for the contribution.