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

refactor(core): Homogenize all providers as static #671

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

zarov
Copy link
Contributor

@zarov zarov commented Feb 27, 2018

Description

There is no reason to keep an instance of each provider, and moving them
to static like some other one (PointCloudProvider for example) do no
harm, but homogenize all providers to one structure.

Note that the function getPointOrder in the WFS provider is unused, should I delete it in this PR, or the associated TODO will be done by someone soon ?

Also note that there are two required functions in a provider:
preprocessDataLayer and executeCommand, that are verified when adding a
provider, in the scheduler.

Motivation and Context

This comes in the work of #182, part of rewriting the Providers/Loader/etc system.

Copy link
Contributor

@autra autra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

I didn't test it though, so if we want this merged before the release, you might want to make sure all the affected examples are still working ok.

if (typeof (provider.preprocessDataLayer) !== 'function') {
throw new Error(`Can't add provider for ${protocol}: missing a preprocessDataLayer function.`);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! That's the first step to really allow people to add their own providers (I mean, it was already possible, but you needed to know the internals of iTowns). Second step would be to document it :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you have in mind for documentation ? Something like iTowns/itowns.github.io#15 ? I can't see where else we could add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe if we document the Scheduler in fact

Copy link
Contributor

Choose a reason for hiding this comment

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

Using http://usejsdoc.org/tags-interface.html#virtual-comments in the Scheduler makes sense.
But maybe you should finish your refactoring commits before adding documentation...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, will do in another PR.

@peppsac
Copy link
Contributor

peppsac commented Feb 27, 2018

Could you remove the unused providers as well (BuildingBox_Provider.js, KML_Provider.js)?

@zarov zarov force-pushed the provider-instance-to-static branch 2 times, most recently from cda22fb to 758b193 Compare February 27, 2018 11:03
@zarov
Copy link
Contributor Author

zarov commented Feb 27, 2018

@autra tested it, all good, but let's wait the 2.3.0 for merging.

@peppsac done.

@peppsac
Copy link
Contributor

peppsac commented Feb 27, 2018

You may want to add a BREAKING CHANGE: ..... statement at the end of your commit message to ease the changelog writing.

@zarov zarov force-pushed the provider-instance-to-static branch 3 times, most recently from 73f1aac to 60dc370 Compare February 27, 2018 11:45
There is no reason to keep an instance of each provider, and moving them
to static like some other one (PointCloudProvider for example) do no
harm, but homogenize all providers to one structure.

Also note that there are two required functions in a provider:
preprocessDataLayer and executeCommand, that are verified when adding a
provider, in the scheduler.

BREAKING CHANGE: KML_Provider and BuildingBox_Provider have been
removed, as well as getPointOrder from WFS_Provider.
@zarov zarov force-pushed the provider-instance-to-static branch from 60dc370 to 17c2546 Compare February 27, 2018 12:06
@peppsac peppsac merged commit a531732 into iTowns:master Feb 27, 2018
@zarov zarov deleted the provider-instance-to-static branch August 29, 2018 08:20
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.

3 participants