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.

Sign up for GitHub

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

Jump to bottom

refactor: introduce commons module #251

Merged
merged 10 commits into from
Oct 16, 2024
Merged

refactor: introduce commons module #251

merged 10 commits into from
Oct 16, 2024

Conversation

ata-nas
Copy link
Contributor

@ata-nas ata-nas commented Oct 15, 2024 •
edited
Loading

Description:

  • introduce :common module that will be used by all other modules
  • the :common module will abstract common logic, mostly constants and utility classes so they could easily be reused and managed
  • DO NOT use this logic in other projects yet, but do it gradually in a few PRs

UPDATE:
After a thorough discussion with the team, a few things were pointed out and agreed on:

  • the :common module should be thought of as an external library in the sense that there should not be any project specific logic there;
  • the module must be at any times able to be extracted as a standalone library, this is the philosophy that should be kept when thinking about the module and what should be added/removed;
  • adding items in this module should be put up to thorough discussion;
  • a few classes that were initially included shall be removed, see comments;

Related issue(s):

Fixes #233

Notes for reviewer:

  1. Have I missed any common logic, any constants or utility classes/methods anywhere?
  2. Let's discuss open comments below.

Checklist

  • create a new :common module
  • abstract all logic that could be reused that fits the philosophy of the module

@ata-nas ata-nas added the Improvement Code changes driven by non business requirements label Oct 15, 2024
@ata-nas ata-nas added this to the 0.2.0 milestone Oct 15, 2024
@ata-nas ata-nas self-assigned this Oct 15, 2024
@ata-nas ata-nas linked an issue Oct 15, 2024 that may be closed by this pull request
refactor: introduce commons module infrastructure #233
Closed
@ata-nas ata-nas changed the title introduce the common module, abstract some literals refactor: introduce the common module, abstract some literals Oct 15, 2024
@ata-nas ata-nas marked this pull request as ready for review October 15, 2024 10:02
@ata-nas ata-nas requested a review from a team as a code owner October 15, 2024 10:02
@ata-nas ata-nas changed the title refactor: introduce the common module, abstract some literals refactor: introduce commons module Oct 15, 2024
georgi-l95
georgi-l95 previously approved these changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@georgi-l95 georgi-l95 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

AlfredoG87
AlfredoG87 requested changes Oct 15, 2024
View reviewed changes
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LG, leaved just some nits and questions.

buildSrc/src/main/kotlin/com.hedera.block.common.gradle.kts Outdated Show resolved Hide resolved
common/build.gradle.kts Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/ErrorMessageConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/ErrorMessageConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/StringConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/StringConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/StringConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/FileUtils.java Outdated Show resolved Hide resolved
AlfredoG87
AlfredoG87 reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

Agreed with comments, thanks for addressing the nits.
What about UTs for this codebase?

common/src/main/java/com/hedera/block/common/constants/StringConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/Translator.java Outdated Show resolved Hide resolved
mattp-swirldslabs
mattp-swirldslabs reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@mattp-swirldslabs mattp-swirldslabs left a comment

Choose a reason for hiding this comment

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

Added comments

buildSrc/src/main/kotlin/com.hedera.block.common.gradle.kts Outdated Show resolved Hide resolved
common/build.gradle.kts Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/constants/StringConstants.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/HashingUtils.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/Translator.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/Translator.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/FileUtils.java Outdated Show resolved Hide resolved
common/src/main/java/com/hedera/block/common/utils/Translator.java Outdated Show resolved Hide resolved
@ata-nas @jsync-swirlds
introduce the common module, abstract some literals
46969a5 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
abstract logic from existing utility classes
ea0d11b 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
abstracting of more logic
bd7798f 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
addressing comment
887e763 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
add todo
e1c9160 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
resolved todo
764bfc4 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
address some comments
6bbfba2 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
address more comments
b0f09e9 
Signed-off-by: Atanas Atanasov <[email protected]>
@ata-nas @jsync-swirlds
address pr comments based on new agreements
0d3475b 
Signed-off-by: Atanas Atanasov <[email protected]>
@jsync-swirlds jsync-swirlds force-pushed the 233-refactor-introduce-commons-module branch from 0ec1107 to 9e76017 Compare October 15, 2024 23:46
@jsync-swirlds
Addressed review and other items
a1723d2 
* Fixed todo item by overloading method and moving default to local constant
* Fixed some formatting issues
* Fixed string utility to match more typical conventions
* Modified "default" file permissions to not make everything executable
* Renamed "abbreviated" classes to use full names
* Renamed unsafe file methods to add "Unsafe" at the end
* Documented why "unsafe" methods are not safe for production use
* General cleanup

Fixed a build headache
* Modified spotless conventions to remove javadoc formatting for now
   * This was creating issues and conflicts between different tools; we'll need
     to add a proper _separate_ javadoc format once we can figure out how to
     do so within the ultra-strict spotless "write a new plugin" approach.

Signed-off-by: Joseph Sinclair <[email protected]>
@jsync-swirlds jsync-swirlds force-pushed the 233-refactor-introduce-commons-module branch from 9e76017 to a1723d2 Compare October 16, 2024 00:20
AlfredoG87
AlfredoG87 approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@AlfredoG87 AlfredoG87 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

common/build.gradle.kts Show resolved Hide resolved
@ata-nas ata-nas merged commit 1e0288c into main Oct 16, 2024
13 checks passed
@ata-nas ata-nas deleted the 233-refactor-introduce-commons-module branch October 16, 2024 16:45
@codecov Codecov
Copy link

codecov bot commented Oct 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.66%. Comparing base (c2a360a) to head (a1723d2).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff            @@
##               main     #251   +/-   ##
=========================================
  Coverage     99.66%   99.66%           
  Complexity      248      248           
=========================================
  Files            51       51           
  Lines           907      907           
  Branches         61       61           
=========================================
  Hits            904      904           
  Misses            3        3

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

@mattp-swirldslabs mattp-swirldslabs mattp-swirldslabs left review comments

@jsync-swirlds jsync-swirlds jsync-swirlds left review comments

@AlfredoG87 AlfredoG87 AlfredoG87 approved these changes

@georgi-l95 georgi-l95 georgi-l95 left review comments

Assignees

@ata-nas ata-nas

Labels
Improvement Code changes driven by non business requirements
Projects
None yet
Milestone
0.2.0
Development

Successfully merging this pull request may close these issues.

refactor: introduce commons module infrastructure
5 participants
@ata-nas @georgi-l95 @mattp-swirldslabs @jsync-swirlds @AlfredoG87