-
Notifications
You must be signed in to change notification settings - Fork 41
Additional types #1334
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
Additional types #1334
Conversation
454a2c9
to
a735397
Compare
It would be good to have a
|
Follow-up from clearlydefined/service#1334 (comment)
Good idea! But I'll handle that in a separate PR. This one has already grown quite large. EDIT: @jeffmendoza ready for review #1336 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the detailed and excellent work! Several of my comments focus on the declaration of private properties or methods in different parts of the code. Let me know if you need any clarification.
private cache: any | ||
|
||
/** Default TTL in seconds for cached items */ | ||
private defaultTtlSeconds: number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider omitting private properties from the declaration, cache and defaultTtlSeconds?
|
||
/** Promise for client initialization to prevent multiple concurrent initializations */ | ||
private _clientReady: Promise<void> | null | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider omitting private properties from the declaration, _client and _clientReady?
@qtomlinson Thank you for the review. I think I've addressed all your comments. The ones I haven't addressed are related to adding types for private methods. I'll try and reply to all of those here, instead of each comment individually. It's not necessary to add types for private fields and methods, but I did it for a few reasons:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JamieMagee Thank you for incorporating the review comments.
Declaring private fields or methods within the types file can reduce readability. For instance, the GradleCoordinatesMapper class contains only two public methods:
map(coordinates: GradleCoordinates): Promise<EntityCoordinates | null>
getMavenMetadata(pluginId: string): Promise<string | FetchResponse<string> | null>
This is easier to understand compared to a scenario where there are 2 public and 8 private methods.
Nonetheless, this is not a critical issue. I appreciate the detailed and excellent changes you've made!
The majority of these types were straightforward to add, and should be self-explanatory to review. However, there were a few places where I had to workaround some issues. I've left comments at the relevant places in the code.
Other than that, I also:
@tsconfig/strictest
strictNullChecks
as it's not really possible to enforce in JavaScript@clearlydefined/spdx
as it doesn't ship with them (I will contribute then back to clearlydefined/spdx afterwards)painless-config
as the types it ships with are incorrect and the upstream repository is archivedwinston-azure-application-insights
as it doesn't ship with them