Remove/gofr dependency s3 #1324
Conversation
…e instead of interface
…fr_dependency_ftp
…into remove/gofr_dependency_s3
| @@ -41,7 +39,7 @@ type jsonReader struct { | |||
| } | |||
|
|
|||
| // ReadAll reads either JSON or text files based on file extension and returns a corresponding RowReader. | |||
There was a problem hiding this comment.
It no longer returns a RowHeader, but an any
| @@ -41,7 +39,7 @@ type jsonReader struct { | |||
| } | |||
|
|
|||
| // ReadAll reads either JSON or text files based on file extension and returns a corresponding RowReader. | |||
| func (f *S3File) ReadAll() (file.RowReader, error) { | |||
| func (f *S3File) ReadAll() (any, error) { | |||
There was a problem hiding this comment.
I have a problem with the any here
Can't you define an interface that jsonReader and csvReader would satisfy?
I don't know at least something with Next, Scan and other if needed.
My main problem is a fact an exported method returns any in its signature, but also unexported structures.
I'm not even sure someone will be able to use the code once refactored
Shouldn't you document things?
There was a problem hiding this comment.
@ccoVeille We can do it maybe but then also if the return type would be file.RowReader or something we have defined in GoFr then we can't remove GoFr's dependency from this datasource.
And even doing it with any is not working here.😐
There was a problem hiding this comment.
Your problems is about dependency import cycle?
Or about depending on external sources, so it leads to add things to gofr go.mod ?
If it's the first one, the solution is often to add a new package where you will move the interface definitions. This package will depends on nothing inside gofr.
Then for compatibility purpose, you replace the existing type at gofr level by aliases to the one at package level.
But here, once again, I might have totally misunderstood what you are trying to achieve
There was a problem hiding this comment.
Or about depending on external sources, so it leads to add things to gofr go.mod ?
We aim to externalize all the datasources. The code in the datasource packages shouldn't depend on GoFr, so we don't have to worry about updating GoFr versions in these packages. This approach also maintains modularity and helps in keeping our binary size light.
However, if we keep the interface definition anywhere inside GoFr, we would need to import it inside the datasource packages for return types, which we want to avoid.
There was a problem hiding this comment.
OK, but having data sources that returns any seems a problem to me.
But maybe other users of gofr could tell what they think about it, maybe a discussion could happen on the discord server
There was a problem hiding this comment.
Can't we move the file.RowHeader and other interface in data sources package?
Another solution is to duplicate the interface definitions which is acceptable.
Then unit tests can be added to validate interface are compatible
Umang01-hash
changed the base branch from
remove/gofr_dependency_ftp
to
development
Umang01-hash
dismissed
aryanmehrotra’s stale review
The base branch was changed.
|
Closing this PR as of now. |
Pull Request Template
Description:
Checklist:
goimportandgolangci-lint.Thank you for your contribution!