Skip to content

commons #93 scalatest uri matcher#94

Merged
cerveada merged 2 commits intomasterfrom
feature/commons-93-scalatest-uri-matcher
Mar 11, 2022
Merged

commons #93 scalatest uri matcher#94
cerveada merged 2 commits intomasterfrom
feature/commons-93-scalatest-uri-matcher

Conversation

@cerveada
Copy link
Contributor

@cerveada cerveada commented Feb 9, 2022

No description provided.

@cerveada cerveada requested a review from wajda February 9, 2022 16:01
@wajda
Copy link
Contributor

wajda commented Feb 9, 2022

Could you also add en example to the Readme please?

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@cerveada
Copy link
Contributor Author

Could you also add en example to the Readme please?

done

Copy link
Contributor

@wajda wajda left a comment

Choose a reason for hiding this comment

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

LGTM

@benedeki
Copy link
Contributor

I was thinking about it. Would say the content of the scalatest folder should be in its own module. e.g. absa-commons-test. This probably shouldn't need to be part of the main branch of the program. 🤔

@wajda
Copy link
Contributor

wajda commented Feb 10, 2022

I was thinking about it. Would say the content of the scalatest folder should be in its own module. e.g. absa-commons-test. This probably shouldn't need to be part of the main branch of the program. thinking

What do you mean by the "main branch of the program"?
Commons is currently a single module project, are you suggesting to break it into multiple modules? We discussed that a while ago, and it certainly would be a logical step forward. There however are about a dozen fine grained packages in the project. We'd need to come up with a sensible break down to modules such that circular dependencies are eliminated and it everything fits together nicely. Provided that there are just one to a few classes per package, do you think it is all worth the efforts?

@benedeki
Copy link
Contributor

I was thinking about it. Would say the content of the scalatest folder should be in its own module. e.g. absa-commons-test. This probably shouldn't need to be part of the main branch of the program. thinking

What do you mean by the "main branch of the program"? Commons is currently a single module project, are you suggesting to break it into multiple modules? We discussed that a while ago, and it certainly would be a logical step forward. There however are about a dozen fine grained packages in the project. We'd need to come up with a sensible break down to modules such that circular dependencies are eliminated and it everything fits together nicely. Provided that there are just one to a few classes per package, do you think it is all worth the efforts?

Yes, multimodule, but not really fine-grained. Only two modules:

  • general usage stuff
  • test specific stuff - like the content of this PR
    The point is, that you probably don't want to include these test classes and functions in the release of the project (the project that includes the commons lib).

@wajda
Copy link
Contributor

wajda commented Feb 10, 2022

Yes, multimodule, but not really fine-grained. Only two modules:

  • general usage stuff
  • test specific stuff - like the content of this PR
    The point is, that you probably don't want to include these test classes and functions in the release of the project (the project that includes the commons lib).

Filed a separate ticket #95.
I completely agree with you, it certainly makes sense. But just to justify my lack of rushing into this, considering that all transient dependencies are marked optional we are only talking about 204Kb of scalatest related bytecode, that in comparison with the size of other packages doesn't make it anything special in that regard. Meaning, for example, if you only use the Commons for calling S3 utils you'd be pulling much more other stuff into your classpath that would be considered useless :)

❯ du -ch target/classes/za/co/absa/commons/ | sort -n -r
340K	target/classes/za/co/absa/commons/config
244K	target/classes/za/co/absa/commons/reflect
216K	target/classes/za/co/absa/commons/version
204K	target/classes/za/co/absa/commons/scalatest
172K	target/classes/za/co/absa/commons/lang
144K	target/classes/za/co/absa/commons/json
96K	target/classes/za/co/absa/commons/json/format
80K	target/classes/za/co/absa/commons/graph
76K	target/classes/za/co/absa/commons/version/impl
56K	target/classes/za/co/absa/commons/reflect/extractors
56K	target/classes/za/co/absa/commons/buildinfo
40K	target/classes/za/co/absa/commons/s3
28K	target/classes/za/co/absa/commons/io
24K	target/classes/za/co/absa/commons/error
16K	target/classes/za/co/absa/commons/annotation
1.6M	total
1.6M	target/classes/za/co/absa/commons/

@benedeki
Copy link
Contributor

Thank for #95,
Just as a closure. I don't mean mostly as size safer, but logical separation. 😉

@cerveada
Copy link
Contributor Author

cerveada commented Mar 1, 2022

@benedeki there is a special ticket for commons fragmentation, is there anything else you want to discuss? If not, could you please approve this PR?

Copy link
Contributor

@dk1844 dk1844 left a comment

Choose a reason for hiding this comment

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

LGTM (just read the code)

@cerveada cerveada merged commit bcb9c41 into master Mar 11, 2022
@cerveada cerveada deleted the feature/commons-93-scalatest-uri-matcher branch March 11, 2022 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants