Skip to content
This repository was archived by the owner on Sep 5, 2023. It is now read-only.

Conversation

@orioljp
Copy link
Contributor

@orioljp orioljp commented Mar 17, 2023

Asana task link:
https://app.asana.com/0/1109863238977521/1204203716532309/f

Pull request information:

  • getAll and putAll methods have been removed from Repositories and DataSources.
  • RawSqlDataSource has been divided in two datasource, each of them with it's own generic type
  • The oauth related code and the examples have been updated to follow this new architecture

@orioljp orioljp requested review from Alex-DA, doup and urijp as code owners March 17, 2023 15:11
Copy link
Contributor

@doup doup left a comment

Choose a reason for hiding this comment

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

There few things you could change, but I'm still approving it. Remember the next releas will need to be 2.0, as these are breaking changes. Kind of unfortunate… as we could have waited before doing the 1.0. 😅

client.clientId,
client.clientSecret,
grants.map((el) => el.grant),
grants.map((el: OAuthClientGrantEntity) => el.grant),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: is this type annotation needed? Isn't correctly inferred from GetDataSource<OAuthClientGrantEntity[]>?

Suggested change
grants.map((el: OAuthClientGrantEntity) => el.grant),
grants.map((el) => el.grant),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@doup Agree, but in a discussion in socialPals @lucianosantana told he preffers that we put explicitly all the types so I'm used to it. We could raise this discussion to MJ.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, is not needed. The type can be inferred quickly on a modern IDE context, but needs a bit of code reading in "dumb" contexts such as browser debugging. I don't think we lose anything by adding that info explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a problem adding it, but it's true that in a dumb context having an OAuthClientGrantEntity you don't have more information than before, and what we have in the end is the logic and algorithms filled with types. My preference is mainly having everything typed but as compact and readable as possible.

Being a library as it is, I'll leave it with these types in there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then this is ready to be merged, right?

);

grants = grantEntities.map((el) => el.grant);
grants = grantEntities.map((el: OAuthClientGrantEntity) => el.grant);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: same here.

Suggested change
grants = grantEntities.map((el: OAuthClientGrantEntity) => el.grant);
grants = grantEntities.map((el) => el.grant);

@orioljp
Copy link
Contributor Author

orioljp commented Mar 27, 2023

@doup The v1.0.0 was really needed in a project so I had to release it. There's nothing wrong on creating a new major release. I had this into account, although we can keep this changes without a new release for now.

Also think that all your suggestions are for things that are not created by me in this PR, just moved from the original place, so some applies no only to the place you pointed out but many others. For these cases I'll create an asana. Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants