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: Refactor data scratch-ipblocks #1811

Merged
merged 9 commits into from Apr 20, 2021
Merged

Conversation

piotradamczyk5
Copy link
Contributor

Fixes #1746

Test Plan

How do we know the code works?

Checklist

  • Unit tested

@github-actions
Copy link
Contributor

github-actions bot commented Apr 15, 2021

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@github-actions
Copy link
Contributor

github-actions bot commented Apr 15, 2021

Timestamp: 2021-04-19 15:56:21
Buildscan url for ubuntu-workflow run 764141388
https://gradle.com/s/emqdhmxeevrb6

@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review April 16, 2021 06:18
Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

The files:

  • IpBlocksToTable.kt
  • ListIPBlocks.kt TestEnvironmentInfo.kt

are not a part of the adapters layer because they don't have any references to google API libs additionally they are widely used in the domain layer. According to the diagram below, the code from adapters cannot be used directly in the domain layer.

architecture diagram

Please move back those two files to the ftl.environment and leave them for further domain refactor.

Copy link
Contributor

@jan-goral jan-goral left a comment

Choose a reason for hiding this comment

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

@piotradamczyk5 Sorry for the mistake, I have copy-pasted the wrong file name, should be TestEnvironmentInfo.kt instead of ListIPBlocks.kt

@piotradamczyk5
Copy link
Contributor Author

The files:

  • IpBlocksToTable.kt
  • ListIPBlocks.kt TestEnvironmentInfo.kt

are not a part of the adapters layer because they don't have any references to google API libs additionally they are widely used in the domain layer. According to the diagram below, the code from adapters cannot be used directly in the domain layer.

architecture diagram

Please move back those two files to the ftl.environment and leave them for further domain refactor.

For me data and adapter layers are badly named:
adapter layer is data while data is adapter

@jan-goral
Copy link
Contributor

@piotradamczyk5 I do not agree. The adapter is an implementation that is adapting the library code or client implementation to the data structures and interfaces declared in the data layer.

@piotradamczyk5
Copy link
Contributor Author

piotradamczyk5 commented Apr 16, 2021

@piotradamczyk5 I do not agree. The adapter is an implementation that is adapting the library code or client implementation to the data structures and interfaces declared in the data layer.

Discussed on slack with @jan-gogo .
The new package structure will be
presentation > domain > api > adapter > client

where
api is old data
old adapter is adapter + client
new adapter prepares and map data
client return data/behaviour from external API (wraps library or library itself)

@piotradamczyk5 piotradamczyk5 marked this pull request as draft April 19, 2021 09:51
@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review April 19, 2021 14:55
@piotradamczyk5 piotradamczyk5 marked this pull request as draft April 19, 2021 15:28
@piotradamczyk5 piotradamczyk5 marked this pull request as ready for review April 19, 2021 15:51
@mergify mergify bot merged commit 3b3bb5a into master Apr 20, 2021
@mergify mergify bot deleted the 1746_refactor_data_ipblocks branch April 20, 2021 11:25
@github-actions github-actions bot locked and limited conversation to collaborators Apr 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data scratch - ip blocks
3 participants