Open to PRs? - Githubissues

joshkopecek commented 8 years ago

I've been doing a bit of work on a GeoJSON schema and I could merge some of the validation work I've been doing?

bendalton commented 8 years ago

Absolutely!

On Fri, Apr 15, 2016, 4:51 AM Josh notifications@github.com wrote:

I've been doing a bit of work on a GeoJSON schema and I could merge some of the validation work I've been doing?

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/7

joshkopecek commented 8 years ago

Ok, I've forked and am making some changes. I'll update you when I get it working.

joshkopecek commented 8 years ago

Hey @bendalton I went a bit crazy. Before I make a pull request I'd like you to see what I've been doing.

Please check out https://github.com/joshkopecek/mongoose-geojson-schema and let me know what you think. I have defined basic tests for Points, and created better validations for most of the schema elements.

joshkopecek commented 8 years ago

Hey @bendalton @markstos I have created a separate branch on my fork partially recreating the schema as a separate mongoose SchemaType.prototype since this seems to be the accepted way to do these things. It would then get attached to mongoose.Schema.Types as separate GeoJSON objects - mongoose.Schema.Types.Point etc. I created 'Point' in this way - take a look and see if you want to proceed down that route - it creates more powerful validation features, although I can't seem to get the custom CastErrors to come up.

markstos commented 8 years ago

Thanks @joshkopecek, we'll take a look.

joshkopecek commented 8 years ago

@bendalton @markstos I've done another commit, converting the rest of the types to SchemaType prototypes. I've written (working) tests for Point, MultiPoint and LineString, the rest should be easy to add.

bendalton commented 8 years ago

This is looking good @joshkopecek! Thanks for working on it.

joshkopecek commented 8 years ago

@bendalton @markstos Nearly there. Just added all the Schema Types and passing tests for everything except Feature and FeatureCollection. Can you check to see what additional tests would be needed for strict adherence to the standard?

joshkopecek commented 8 years ago

@bendalton @markstos I've finished adding Feature and FeatureCollection plus tests for both. I also updated the readme. Let me know what you think.

markstos commented 8 years ago

@joshkopecek, Here's some feedback:

The original defined an overall "Geometry" type, but the fork does not. Could you add it?
As discussed, validating points as lat/lon values is valuable, but not spec compliant. The validation behavior should be optional and documented.
Speaking of documentation, we need some. :) The documentation should explain what the GeoJSON schemas are doing and any constraints they adding.
Another way to validate your work is to check the other open issues and see it addresses any of them.
It would be great if "npm test" worked as a standard command. I just made this work in the main branch. Here's the line in the package.json to make it work: "test": "./node_modules/mocha/bin/mocha test/*"

I see that the syntax is changing from 0.x to 1x to do things in a standard way. That's fine.

I see you added a lot of test coverage and linting. Thanks!

joshkopecek commented 8 years ago

Thanks @markstos for the feedback. Still some work to do here then.

Will add Geometry
Will remove Point boundary validation to comply with strict GeoJSON schema
I'll have a look at the documentation and see what I can add. I guess it should just explain that it embodies the GeoJSON spec.
I'll have a look at the open issues :-)
npm test is an easy mod

I was considering an uglify/minification build to dist, but I guess people can do this in their main project along with concat anyway so it doesn't achieve a lot.

markstos commented 8 years ago

Thanks @joshkopecek. You are on the way to being offer commit and publish access.

As far as I'm aware Mongoose only runs in the server side, so I don't think there'd be a benefit to minify the code there.

Your help is much appreciated.

joshkopecek commented 8 years ago

Hey @markstos I've finished the changes. I had a look at issues but they won't all be relevant to the new structure, for example it'll be up to the person making the schema to add their own indexes (issue #5) and the tests contain support for properties in Features (issue #6 and #2). It fixes #4 and #3.

Let me know what you think.

markstos commented 8 years ago

Thanks! I'll take another look.

On Tue, Apr 26, 2016 at 9:33 AM, Josh notifications@github.com wrote:

Hey @markstos https://github.com/markstos I've finished the changes. I had a look at issues but they won't all be relevant to the new structure, for example it'll be up to the person making the schema to add their own indexes (issue #5 https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/5) and the tests contain support for properties in Features (issue #6 https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/6 and

2 https://github.com/RideAmigosCorp/mong%20oose-geojson-schema/pull/2).

It fixes #4 https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/4 and

3 https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/3.

Let me know what you think.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/7#issuecomment-214746199

Mark Stosberg
Senior Systems Engineer
RideAmigos

markstos commented 8 years ago

@joshkopecek Good news regarding validating coordinates.

It turns out that by default GeoJSON does not lack a coordinate system, rather "The default CRS is a geographic coordinate reference system, using the WGS84 datum, and with longitude and latitude units of decimal degrees.".

The way an alternate coordinate system is specified is by providing a 'crs' property in the GeoJSON object. The spec says: "A GeoJSON object may have an optional "crs" member, the value of which must be a coordinate reference system object".

So to reverse what I said previously, I think it /is/ spec-compliant and appropriate to validate latitude and longitude. I think this is what most people would want, so I'm glad we can include it as a default and be spec-compliant as well.

The caveat that would be nice to implement is to look for the crs property in the GeoJSON objects. If it is present, turn off validation, because we don't know to validate the coordinates for other coordinate systems.

Overall, things are looking great.

joshkopecek commented 8 years ago

@markstos thanks for the help and pointers for the CRS stuff.

Added CRS validation if crs is present. Checks for 'name' or 'link' objects and verifies they're correct. If CRS is present it disables point validation, otherwise checks against the specs I build before.

I think the strict compliance is definitely getting there.

https://github.com/joshkopecek/mongoose-geojson-schema

markstos commented 8 years ago

@joshkopecek,

I sent a PR to your branch with some refinements. Here's what I found when reviewed the code:

Good test coverage, thanks.
I see that whether crs is present is cached in the module, rather than explicitly passed around. I was concerned this could to an accidental caching problem. Where a first use enables CRS, and a second use does not, but yet CRS is enabled from the previous call. However, I did not find the bug to actually exist and also found that the case was already covered in the test suite. So my concern here was addressed.

I'm curious if we can improve the exceptions thrown any more. In the code, there are nice specific exception messages given.

However, when I reviewed the exceptions actually thrown in the test suite, they are generic, like "failed to cast value to Point", but the specific reason isn't given. Is there a way for people to see exactly /why/ validation fails for a particular value?

It seems odd that the Mongoose system would allow defining these specific exception strings, but then return a generic "failure to cast" message when there's actually a validation problem.

Overall, this looks publish-ready, I'm just curious if we can improve the diagnostics further.

joshkopecek commented 8 years ago

@markstos

The crs object just needs to be passed through the functions, and yes I saw the caching problem. So that can be addressed in an optimisation.

Regarding the exceptions, I was at a loss as to why after I'd carefully written mongoose.SchemaType.CastError()s for all of them, mongoose was returning the generic 'failed to cast value…' exceptions.

It quite clearly states here that you should be able to catch the 'message', but I was just getting a generic return. If you try logging one of the errors in the GeoJSON.integration file you'll see what I mean.

I originally thought it was because I'd not provided both arguments to CastError(), but providing two doesn't help.

throw new mongoose.SchemaType.CastError('Point', 'Point must have a type');

still results in the same generic CastError. Pulling my hair out here. Any ideas?

markstos commented 8 years ago

I'll ask in the Mongoose Github tracker.

joshkopecek commented 8 years ago

@markstos tag me if you can, thanks.

markstos commented 8 years ago

Here's the post: https://groups.google.com/forum/#!topic/mongoose-orm/uISzPQYq8Eg

joshkopecek commented 8 years ago

No reply yet. Is it worth opening an issue on mongoose github?

markstos commented 8 years ago

@joshkopecek I think opening an Mongoose issue is fair if you want to do that. I couldn't find authorative documentation for CastError but perhaps I missed something. It certainly seems like a bug to have a API that allows developers to provide specific error messages, and then not have a way to surface those error messages.

It might also be interesting just read the Mongoose source code where CastError is mentioned, to try to understand the intent of CastError was meant to work.

joshkopecek commented 8 years ago

Posted an issue on mongoose here

I looked at the code for CastError, which seems to suggest that you could specify 4 params but I tried it and still results in undefined for 'reason' and the generic error message.

If you mean this in terms of documentation for CastError it leaves a lot to be desired.

markstos commented 8 years ago

@joshkopecek at thiis point, it seems clear enough that improving the CastError display is largely an upstream issue-- either an upstream bug-fix or doc-fix will solve the issue.

Are you ready to release what's there now and defer the improved CastError formatting for later?

You are also welcome to take over the primary maintainer if you'd like and have npm publish rights to the namespace, if that interests you.

joshkopecek commented 8 years ago

@markstos let's leave the CastError problem. I'll keep an eye on the issues on mongoose, perhaps we can get some clarification.

Very happy to release if you're ready.

If you want to transfer npm publish rights that's up to you, I'm happy to take over as primary maintainer. Although please don't disappear! I'll probably need help after it's released ;-)

joshkopecek commented 8 years ago

Want me to open a PR?

markstos commented 8 years ago

@joshkopecek, no need for a PR. You have direct commit access now. Also, now have publish rights on the 'mongoose-geojson-schema name space, so you are all set to finish the release.

I'll continue to be around to help. I expect we'll be using Mongoose+GeoJSON for a good while longer.

bendalton commented 8 years ago

@joshopecek - thanks for contributing. Tons of great stuff here. Thanks @markstos for coordinating.

On Wed, May 4, 2016 at 10:54 AM Mark Stosberg notifications@github.com wrote:

@joshkopecek https://github.com/joshkopecek, no need for a PR. You have direct commit access now. Also, now have publish rights on the 'mongoose-geojson-schema name space, so you are all set to finish the release.

I'll continue to be around to help. I expect we'll be using Mongoose+GeoJSON for a good while longer.

— You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/7#issuecomment-216890372

joshkopecek commented 8 years ago

@markstos @bendalton Thanks for all your help guys. Committed and published. I'll close this issue now!

echoes-xyz / mongoose-geojson-schema

Open to PRs? #7

2 https://github.com/RideAmigosCorp/mong%20oose-geojson-schema/pull/2).

3 https://github.com/RideAmigosCorp/mongoose-geojson-schema/issues/3.