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

GeoServer REST importer #285

Merged
merged 11 commits into from
Nov 29, 2017
Merged

GeoServer REST importer #285

merged 11 commits into from
Nov 29, 2017

Conversation

dnlkoch
Copy link
Member

@dnlkoch dnlkoch commented Nov 24, 2017

This adds the GeoServerRESTImporter that can be used to easily call the GeoServer importer extension's REST API.

@dnlkoch dnlkoch changed the title GeoServer REST importer util GeoServer REST importer Nov 24, 2017
@ahennr ahennr requested review from chrismayer, KaiVolland, buehner and ahennr and removed request for chrismayer November 24, 2017 13:33
Copy link
Member

@ahennr ahennr left a comment

Choose a reason for hiding this comment

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

Awesome work @dnlkoch. I've just added some minor issues only (mostly regarding Javadoc comments)

/***
*
* @param importerBaseURL
* @param defaultSRS
Copy link
Member

Choose a reason for hiding this comment

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

not needed

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in b119a96.


/**
*
* @param importJob
Copy link
Member

Choose a reason for hiding this comment

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

wrong javadoc parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in b119a96.

* @author terrestris GmbH & Co. KG
*
*/
public class RESTDataDirectory extends RESTData {
Copy link
Member

@ahennr ahennr Nov 24, 2017

Choose a reason for hiding this comment

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

Is this class currently needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, actually not. I would leave it for future purposes, ok?

Copy link
Member

Choose a reason for hiding this comment

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

Alright.

* @author terrestris GmbH & Co. KG
*
*/
public class RESTDataFile extends RESTData {
Copy link
Member

@ahennr ahennr Nov 24, 2017

Choose a reason for hiding this comment

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

Is this class currently needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

* @author terrestris GmbH & Co. KG
*
*/
public class RESTDataRemote extends RESTData {
Copy link
Member

@ahennr ahennr Nov 24, 2017

Choose a reason for hiding this comment

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

Is this class currently needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

* @author terrestris GmbH & Co. KG
*
*/
public class RESTDateFormatTransform extends RESTTransform {
Copy link
Member

@ahennr ahennr Nov 24, 2017

Choose a reason for hiding this comment

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

Is this class currently needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

See above.

@@ -193,6 +197,12 @@
<!-- Apache HTTP Client -->
<apache-httpclient.version>4.5.3</apache-httpclient.version>

<!-- GeoTools -->
<geotools.version>17.3</geotools.version>
Copy link
Member

Choose a reason for hiding this comment

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

Does this version depend on the used version of GeoServer resp. the used version of the importer?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, actually not.

Copy link
Member

@KaiVolland KaiVolland left a comment

Choose a reason for hiding this comment

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

Some more documentation on the properties would be nice.
I'd also prefer to not submit the dummy classes. But my opinion on this is not very strong.

All in all nothing blocking. So feel free to merge.

/**
*
*/
private URI baseUri;
Copy link
Member

Choose a reason for hiding this comment

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

PLz provide some more docs.

@ahennr
Copy link
Member

ahennr commented Nov 29, 2017

Will merge this PR now and create an issue for enhancement of the docs

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