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

Make "layers" parameter of IExportParameters optional #775

Merged
merged 3 commits into from
Nov 11, 2020
Merged

Conversation

rgwozdz
Copy link
Contributor

@rgwozdz rgwozdz commented Nov 6, 2020

We want the option of not setting layers in IExportParameters. That will force the /export service to include all the layers in the delivered export.

…ters\` optional

AFFECTS PACKAGES:
@esri/arcgis-rest-portal
@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #775 (f8bb857) into master (ed3e37f) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #775   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          120       120           
  Lines         1915      1915           
  Branches       325       325           
=========================================
  Hits          1915      1915           
Impacted Files Coverage Δ
packages/arcgis-rest-portal/src/items/export.ts 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed3e37f...e91f3e0. Read the comment docs.

@tomwayson
Copy link
Member

But this says layers is required: https://developers.arcgis.com/rest/users-groups-and-items/export-item.htm - in fact, that appears to be the only parameter on that page that's called out as required?

@rgwozdz
Copy link
Contributor Author

rgwozdz commented Nov 9, 2020

@tomwayson - it does say that, but in fact, it is not true. I've tested it and can send you the curl if you like. There are cases where you want to export all layers but still set a targetSR. In such cases, you would leave out the layers parameter.

Copy link
Member

@tomwayson tomwayson left a comment

Choose a reason for hiding this comment

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

only suggestion would be to put this into a code comment:

#775 (comment)

@rgwozdz
Copy link
Contributor Author

rgwozdz commented Nov 9, 2020

Confirmed that the docs are out of date; will add code-comment to note the discrepancy.

@dbouwman
Copy link
Member

dbouwman commented Nov 9, 2020

@rgwozdz fyi - CI fails on node 15.1.0 due to npm/cli#2084
My PR will remove latest node from running in CI until that issue is resolved... that should merge later today...

@rgwozdz
Copy link
Contributor Author

rgwozdz commented Nov 10, 2020

@dbouwman - would it help if I do a separate PR for removing latest Node from Travis?

@rgwozdz rgwozdz merged commit c085217 into master Nov 11, 2020
@rgwozdz rgwozdz deleted the p/export-item branch November 11, 2020 19:18
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.

4 participants