Skip to content

Conversation

@0xced
Copy link
Contributor

@0xced 0xced commented Jun 1, 2023

What does this PR do?

This PR extracts all the common properties across all test projects (except for Testcontainers.Commons) into the Directory.Build.props file.

Why is it important?

This reduces duplication and makes it easier to maintain test projects.

Related issues

None

By sharing common properties in the Directory.Build.props file.
@netlify
Copy link

netlify bot commented Jun 1, 2023

Deploy Preview for testcontainers-dotnet ready!

Name Link
🔨 Latest commit 3b3f3cc
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-dotnet/deploys/6478e2be0bbe6b0008f8cb37
😎 Deploy Preview https://deploy-preview-908--testcontainers-dotnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Collaborator

@HofmeisterAn HofmeisterAn left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your pull request. In general, it is recommended to discuss pull requests beforehand to minimize unnecessary work (Contributing). I am aware of the current configuration (copy and paste), and it is intentionally.

We are considering the implementation of a multi-repo approach, which is why I made an effort to ensure that the module dependencies remain as autonomous as possible. THB, I am uncertain how to continue here. For a single-repo approach the changes make sense /cc @kiview.

@0xced
Copy link
Contributor Author

0xced commented Jun 2, 2023

I read the contributing guide but felt that this pull request did not fall into the anything big category. It may look big by the number of files involved but it was merely a search and replace on multiple files. 😉

About the multi-repo approach, I'm not sure it would be a benefit for this project. With 16 open issues and 8 open pull requests it still seems manageable. But I'm not a maintainer so of course I'll let this decision up to you.

@HofmeisterAn
Copy link
Collaborator

I spoke with the other TC maintainers, and right now, we do not want to increase coupling across modules, etc. We prefer self-contained configurations.

We really appreciate your help, and it is difficult for me to not accept your PR. Your changes are fine from a technical standpoint, and I understand the intention. But we do not want to make any changes in this direction yet.

@0xced
Copy link
Contributor Author

0xced commented Jun 12, 2023

The amount of code duplication still itches me but I understand your decision.

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.

2 participants