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

Remove type casts in example sources for display #3537

Closed
wants to merge 1 commit into from
Closed

Remove type casts in example sources for display #3537

wants to merge 1 commit into from

Conversation

marcjansen
Copy link
Member

This PR suggests that we remove inline type casts when we display our examples.

Before:

before-with-typecast

After:

after-no-typecast

@marcjansen
Copy link
Member Author

I'm not sure if this really does the trick for all cases...

*/
function cleanupScriptSource(src) {
// The regex to replace … goog.require … and … renderer: exampleNS. … lines
var lineRegEx = /.*(goog\.require(.*);|.*renderer: exampleNS\..*,?)[\n]*/g;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be defined outside the function to avoid RegEx parsing with every function call.

@ahocevar
Copy link
Member

Looks good - maybe you'll want to address my two minor comments before merging. Even if the regex does not catch all cases, it is a good starting point for later improvements.

@marcjansen
Copy link
Member Author

Thanks @ahocevar. I want to test the results first again... It may be that the regex chnges code i don't want it to change.

@marcjansen
Copy link
Member Author

The regex fails to clean up e.g.

view.fitExtent(
      vectorSource.getExtent(), /** @type {ol.Size} */ (map.getSize()));

and turns it into

view.fitExtent(
      vectorSource.getExtent(), );

(examples_src/drag-and-drop-image-vector.js)

Other files with problematic constructs are animation.js or measure.js:

var geom = /** @type {ol.geom.Polygon} */(polygon.clone().transform(
        sourceProj, 'EPSG:4326'));

It may be that the regular expression can be enhanced, but I fear this is going to be hard.

A) I could limit the regex to the attribution options (olx.control.AttributionOptions), which would remove most of the typecasts
B) or the regexp could be changed so it only removes the typecast comment and leaves additional (useless) round parenthesises:

var geom = /** @type {ol.geom.Polygon} */(polygon.clone().transform(
        sourceProj, 'EPSG:4326'));
var geom = (polygon.clone().transform(
        sourceProj, 'EPSG:4326'));

C) or I close this PR as fruitless.

Opinions?

@marcjansen
Copy link
Member Author

B) would most probably be the easiest way.

@ahocevar
Copy link
Member

I think both B and C would be fine.

@marcjansen
Copy link
Member Author

Hah, I think I have an easy solution that'll do all I originally intended... I'll see if I can rework this tomorrow.

@marcjansen
Copy link
Member Author

I went for option B as suggested. I think this still is a nice little enhancement and one thing less that may confuse first-time readers. The surrounding round braces stay now, but they do not harm. All problematic cases of the original commit are no longer an issue.

@ahocevar your other remarks are adressed, thanks for your review.

Hah, I think I have an easy solution that'll do all I originally intended... I'll see if I can rework this tomorrow.

I am no longer sure if I can come up with something that also does remove the ( and ).

Anybody willing to give this another look? TIA.

// opening curly brackets, any non-whitespace characters, closing curly brace,
// an optional space, the end of a multiline comment followed by any number of
// whitespace characters
var cleanTypecastRegEx = /(\/\*\* ?@type \{\S*\} ?\*\/\s*)/g;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The group is not really needed here.

@elemoine
Copy link
Member

I might have concerns with this patch, so I'd like to review it before it is merged. Thanks.

@elemoine
Copy link
Member

I had misunderstood this patch. I thought it changed the examples' code, while it changed the code that is displayed in the example's page!

That being said, option B (remove typecast but leave brackets) sounds like a half-baked solution to me. So I'd either address the issue completely or go with option C (close this PR as fruitless).

@marcjansen
Copy link
Member Author

No problem. I'll close it then. Any ideas howwe could accomplish this?

@marcjansen marcjansen closed this Apr 14, 2015
@marcjansen marcjansen deleted the examples-no-type-casts branch March 21, 2016 16:29
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.

None yet

3 participants