Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type rewrite further improvement #11

Merged

Conversation

simonseyock
Copy link
Member

@simonseyock simonseyock commented Oct 4, 2019

Fixes openlayers/openlayers#10075

Because regex are inherently bad at matching nested structures and I had fun in digging into grammar and especially pegjs (https://pegjs.org/), I wrote a grammar to perform the type rewrites. I also made a branch (#12), but I don't know if this should be merged because it would introduce a new syntax that might be difficult to maintain if not known and a new dependency.

The parser approach has 2 big advantages: It can easily handle nested structures and it can throw errors if it finds expressions that it should be able to parse, but some type is missing.

Nevertheless I used this grammar to check the output of our regular expression and found several issues. I created a new regex that does not produce any false positives and only misses a handful of rewrites in the whole code base.

Examples for expressions it can not handle are multiline types and some strangely nested types.

  * @typedef {function(!SketchCoordType, SimpleGeometry=):
  *     SimpleGeometry} GeometryFunction
  * @param {{type: string,
  *     target: (EventTargetLike|undefined),
  *     propagationStopped: (boolean|undefined)}|
  *     BaseEvent|string} event Event object.
  * @type {!Object<string, (Array<Feature>|Feature|undefined)>} 

I also found some more incorrect annotations in the codebase, that I fix in PR on the main repository.

@simonseyock simonseyock mentioned this pull request Oct 4, 2019
@simonseyock
Copy link
Member Author

Also I found some types that where annotated like this: {module:ol/ImageState} which I don't know if they are correct. I would interpret these as the default export? I did not find something definitive in the jsdoc documentation.

@simonseyock
Copy link
Member Author

simonseyock commented Oct 4, 2019

One more thing is that @enum declarations are not detected as local types.

@simonseyock simonseyock changed the title Type rewrite further improvement WIP: Type rewrite further improvement Oct 4, 2019
@simonseyock
Copy link
Member Author

simonseyock commented Oct 9, 2019

I added @enum rewriting and am now quite certain that this version works with all jsdoc comments except of 7.

Apart from the ones above these are:

 * @typedef {function(module:ol/Feature~FeatureLike, number):(Style|Array<Style>)} StyleFunction
 * @typedef {Style|Array<Style>|StyleFunction} StyleLike
 * @param {StyleFunction|Array<Style>|Style} obj
* @typedef {UniformLiteralValue|function(module:ol/PluggableMap~FrameState):UniformLiteralValue} UniformValue

@simonseyock simonseyock changed the title WIP: Type rewrite further improvement Type rewrite further improvement Oct 9, 2019
@simonseyock
Copy link
Member Author

@ahocevar can you look at this?

@ahocevar
Copy link
Member

ahocevar commented Oct 9, 2019

@simonseyock Thanks for your work on this. I think the nested ones you listed above fail because they are multi-line comments. Maybe putting them on a single line would make them work? And the last one you mentioned has an invalid type (module:ol/PluggableMap~FrameState).

So I think you did a great job with this. Thanks!

@ahocevar ahocevar merged commit f247302 into openlayers:master Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Animation options not listed in API
2 participants