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

Adjusted Network Object and Data Provider Types #163

Merged
merged 6 commits into from
May 1, 2023

Conversation

tjcouch-sil
Copy link
Member

@tjcouch-sil tjcouch-sil commented Apr 28, 2023

Adjusted network object and data provider types to indicate their constraints and relationships more strongly.

Renamed Data Provider types/interfaces:

IDataProviderEngine [stayed the same] (like NetworkableObject)
IDataProvider -> DataProviderInternal (like NetworkableObject)
DisposableDataProvider -> IDisposableDataProvider (like DisposableNetworkObject)
DataProvider -> IDataProvider (like NetworkObject)

Relates to #125


This change is Reviewable

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)


src/shared/models/network-object.model.ts line 4 at r1 (raw file):

// This is used in @see comments in the JSDoc
// eslint-disable-next-line @typescript-eslint/no-unused-vars
import type networkObjectService from '@shared/services/network-object.service';

Did you have a problem with the comments if the type wasn't imported?

Technically I think I did this wrong, and I probably should have used a @link instead of @see. It isn't that important at least until we do something more substantial with JSDoc.

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tjcouch-sil)

Copy link
Contributor

@katherinejensen00 katherinejensen00 left a comment

Choose a reason for hiding this comment

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

Note, there are some comments that are too long in papi.d.ts, but I don't think that is worth fixing. This looks good to me. Thanks for all of the work!

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tjcouch-sil)


lib/papi-dts/papi.d.ts line 119 at r1 (raw file):

  /**
   * HTML Encodes the provided string.
   * Thanks to ChatGPT

That's cool :)


lib/papi-dts/papi.d.ts line 2101 at r1 (raw file):

          eventHandler: import('shared/models/papi-event.model').PapiEventHandler<T_1>,
        ) => void;
        useEventAsync: <T_2>(

Just a question to make sure I understand, do you need T_1, T_2, etc. so that T_1 can be a separate type from T_2?

Copy link
Member Author

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @katherinejensen00)


lib/papi-dts/papi.d.ts line 119 at r1 (raw file):

Previously, katherinejensen00 wrote…

That's cool :)

Can't take credit; this was Matt's idea :)


lib/papi-dts/papi.d.ts line 2101 at r1 (raw file):

Previously, katherinejensen00 wrote…

Just a question to make sure I understand, do you need T_1, T_2, etc. so that T_1 can be a separate type from T_2?

papi.d.ts is a generated file, so the comments are long and this is happening. There are a number of weird things in this file that would be good to improve, but I don't really know how to fine-tune it right now unfortunately. Would be good to investigate. #166


src/shared/models/network-object.model.ts line 4 at r1 (raw file):

Previously, lyonsil wrote…

Did you have a problem with the comments if the type wasn't imported?

Technically I think I did this wrong, and I probably should have used a @link instead of @see. It isn't that important at least until we do something more substantial with JSDoc.

Not sure why, but I got an error at one point when this wasn't in there. But then after I put it in, I tried taking it out again and didn't see the problem after that. Not sure what's going on 🤷‍♂️

@tjcouch-sil tjcouch-sil enabled auto-merge May 1, 2023 14:24
Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @tjcouch-sil)

@tjcouch-sil tjcouch-sil merged commit c70c99b into main May 1, 2023
@tjcouch-sil tjcouch-sil deleted the 125-info-removal-tweaks branch May 1, 2023 14:38
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