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.

Sign up for GitHub

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

Jump to bottom

Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-feature-service") #930

Merged
merged 6 commits into from
Nov 15, 2021
Merged

Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-feature-service") #930

merged 6 commits into from
Nov 15, 2021

Conversation

gavinr
Copy link
Contributor

@gavinr gavinr commented Oct 22, 2021

follow-up to #926

@gavinr-maps
renamed "arcgis-rest-feature-layer"
b4df4d6 
renamed "arcgis-rest-feature-layer" to "arcgis-rest-features"
@gavinr gavinr changed the title Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-feature-layer") Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-features") Oct 22, 2021
@gavinr gavinr marked this pull request as ready for review October 22, 2021 16:01
patrickarlt
patrickarlt approved these changes Oct 22, 2021
View reviewed changes
Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

LGTM. Not sure if we want to name this arcgis-rest-feature-service or arcgis-rest-features. arcgis-rest-features feels a little oddly general to me. If we wanted to include a separate package for map services would we callit arcgis-rest-map?

@patrickarlt
Copy link
Contributor

@gavinr does the new package need a dependency on portal for the admin stuff?

@gavinr
Copy link
Contributor Author

gavinr commented Oct 22, 2021

@patrickarlt yes, I'll look into it - something else is missing too since the unit tests are failing. I'll look into that.

Re: naming, I was going off #926 (comment) but can definitely change it if we want to have more discussions on it.

@codecov
Copy link

codecov bot commented Oct 26, 2021 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (v4.0@a9590bf). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             v4.0      #930   +/-   ##
========================================
  Coverage        ?   100.00%           
========================================
  Files           ?       137           
  Lines           ?      2444           
  Branches        ?       427           
========================================
  Hits            ?      2444           
  Misses          ?         0           
  Partials        ?         0

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 a9590bf...9950fe3. Read the comment docs.

@gavinr
Copy link
Contributor Author

gavinr commented Oct 26, 2021

I added that dependency and the unit tests are now passing.

Should we change the name to arcgis-rest-feature-service?

@patrickarlt
Copy link
Contributor

@gavinr yes I would rename this to arcgis-rest-feature-service and resolve the merge conflicts.

@gavinr
Copy link
Contributor Author

gavinr commented Nov 10, 2021

ok, I'll rename it and resolve conflicts to try and get this merged soon. Thanks!

@gavinr-maps
Merge branch 'v4.0' into v4.0-merge-packages-arcgis-rest-features
edac243 
# Conflicts:
#	demos/attachments-browser/index.html
#	demos/feature-service-browser/index.html
#	package-lock.json
#	packages/arcgis-rest-features/README.md
#	packages/arcgis-rest-features/test/addToServiceDefinition.test.ts
#	packages/arcgis-rest-features/test/getViewSources.test.ts
#	packages/arcgis-rest-features/test/updateServiceDefinition.test.ts
#	packages/arcgis-rest-service-admin/README.md
@gavinr-maps
fix from merge
a38ae99
@gavinr-maps
rename package to "arcgis-rest-feature-service"
9950fe3
@gavinr gavinr changed the title Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-features") Merge "arcgis-rest-feature-layer" and "arcgis-rest-service-admin" together (renamed "arcgis-rest-feature-service") Nov 12, 2021
patrickarlt
patrickarlt approved these changes Nov 12, 2021
View reviewed changes
Copy link
Contributor

@patrickarlt patrickarlt left a comment

Choose a reason for hiding this comment

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

LGTM!

@gavinr gavinr merged commit 5dee521 into Esri:v4.0 Nov 15, 2021
@gavinr gavinr deleted the v4.0-merge-packages-arcgis-rest-features branch November 15, 2021 14:40
@gavinr-maps gavinr-maps mentioned this pull request Apr 5, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickarlt patrickarlt patrickarlt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gavinr @patrickarlt @gavinr-maps