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 feature-layer/ I*Params to I*Options #521

Merged
merged 3 commits into from
Apr 18, 2019

Conversation

tomwayson
Copy link
Member

more consistent naming for interfaces in the feature-layer package

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-layer

BREAKING CHANGE:
renamed IEditFeaturesParams to ISharedEditOptions & ISharedQueryParams to ISharedQueryOptions;
removed IDeleteFeaturesParams

more consistent naming for interfaces in the feature-layer package

AFFECTS PACKAGES:
@esri/arcgis-rest-feature-layer

BREAKING CHANGE:
renamed IEditFeaturesParams to ISharedEditOptions & ISharedQueryParams to ISharedQueryOptions;
removed IDeleteFeaturesParams
@tomwayson tomwayson requested a review from jgravois April 18, 2019 00:09
@tomwayson
Copy link
Member Author

addresses #520 (review)

@tomwayson
Copy link
Member Author

while doing this I noticed that IDeleteFeaturesResult is inconsistent w/ the other I*Response interfaces

image

but I assume we're going to get rid of of all of those as per #137, so I didn't address that in this PR.

Speaking of, do you want me to do ^^^ for the feature-layer package, @jgravois? I assume it would be a matter of just removing the interface and inlining the shape of the response into the function declaration's Promise<>, like:

export function deleteFeatures(
  requestOptions: IDeleteFeaturesRequestOptions
): Promise<{ deleteResults?: IEditFeatureResult[]; }> {
// instead of Promise<IDeleteFeaturesResult>

right?

@jgravois
Copy link
Contributor

right?

right.

@COV-GIS
Copy link
Contributor

COV-GIS commented Apr 18, 2019

@jgravois @tomwayson the reason add, update and delete had different result interfaces is ags returns different for each. i haven't been following the rework closely so maybe it doesn't matter. but if there is going to be some sort of shared result interface just be aware the result is different.

@tomwayson
Copy link
Member Author

but if there is going to be some sort of shared result interface just be aware the result is different.

No shared interface, just each fn will declare inline what it returns.

@COV-GIS
Copy link
Contributor

COV-GIS commented Apr 18, 2019

Good deal.

On hold starting a couple new projects to get up to speed on v2. I appreciate all the debate and hard work for my benefit @tomwayson @jgravois @patrickarlt. :)

@tomwayson
Copy link
Member Author

sounds like we've got our first beta tester!

@tomwayson
Copy link
Member Author

response interfaces removed in #523

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

i'm loathe to point this out, but my OCD rules everything around me.

historically we've used IWhateverRequestOptions. I actually prefer ISharedQueryOptions over ISharedQueryRequestOptions though. The length of the interface names I wrote has always annoyed me.

that said, i don't think dropping Request from all the other interfaces isn't worth the PITA it would entail either.

All this to say, whatever you decide to do here, i'm behind you 100%.

@@ -19,7 +19,7 @@ export interface ILayerRequestOptions extends IRequestOptions {
url: string;
}

export interface ISharedQueryParams {
export interface ISharedQueryOptions {
Copy link
Contributor

Choose a reason for hiding this comment

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

i was thinking this:

export interface ISharedQueryOptions extends ILayerRequestOptions {

so that you could do

export interface IAddFeaturesRequestOptions
  extends ISharedEditOptions {

@tomwayson
Copy link
Member Author

Yeah, I'd love to pull "Request" from all the options. I don't think the work would be bad, it would just be the fact that we would be moving all the 🧀 , which shouldn't affect too many users. I would guess that even TS users pass POJOs to the fns most of the time.

@jgravois
Copy link
Contributor

I'd love to pull "Request" from all the options.

its a wonder how either of us gets any real work done.

@tomwayson
Copy link
Member Author

I don't think that we do.

@patrickarlt
Copy link
Contributor

@jgravois I'm going to make a sticker of this:

my OCD rules everything around me.

@tomwayson tomwayson merged commit 7a3a47c into v2.0.0 Apr 18, 2019
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