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

Share Guard code across repositories #2

Merged
merged 7 commits into from
Aug 31, 2019
Merged

Conversation

antonfirsov
Copy link
Member

@antonfirsov antonfirsov commented Aug 18, 2019

Currently Guard and DebugGuard is being shared as an internal of SixLabors.Core. This could lead to compatibility issues.

I'm suggesting to share the code the code instead with the help of already existing "Standards" submodule. The classes are tested within the bounds of the SixLabors.Core repository.

Parallel PR-s to apply the change in across repositories

@antonfirsov antonfirsov changed the title Share Guard code across projects Share Guard code across repositories Aug 18, 2019
Copy link
Member

@JimBobSquarePants JimBobSquarePants left a comment

Choose a reason for hiding this comment

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

@antonfirsov
Copy link
Member Author

@JimBobSquarePants that would mean introducing yet another infrastructure element raising where/how +maintenance questions. Submodules are already in place for our stuff, and looking simpler to me (no need to publish on myget etc.).

@antonfirsov
Copy link
Member Author

If we find it's worth the efforts, we can repackage it later anytime. For now I just want to get rid of those dangerous dependencies for the next beta.

@JimBobSquarePants
Copy link
Member

Those projects don’t get deployed, only their source is shared to the consuming projects so package management is not a factor. We can also enforce standards rules on the shared code.

I’d much rather introduce a new submodule here anyway rather than pollute standards, or replace standards with something more aptly named if we are to use it as a general store.

@antonfirsov
Copy link
Member Author

antonfirsov commented Aug 29, 2019

@JimBobSquarePants I've renamed the repository, and changed README.md according to it's new purpose.

Now it needs an extensive review together with (updated) SixLabors/Core#33, before so we can do the rest of the work. I'll change and finalize the remaining PR-s, as soon as this one is merged.

@JimBobSquarePants
Copy link
Member

@antonfirsov I've just pushed an update that uses the most up-to-date .editorconfig (plus VS 16.2.2 fix) and .gitattributes so we can share them across our repos also.

Additionally I've reordered the SixLabors.ruleset config for readability and added a new SixLabors.Tests.ruleset that contains a slimmed down ruleset more suitable for our unit tests.

The readme has been updated to reflect those changes.

I still think we should use shared projects in the future over standalone .cs files but that can wait for future discussion.

@JimBobSquarePants JimBobSquarePants merged commit 224ad30 into master Aug 31, 2019
@JimBobSquarePants JimBobSquarePants deleted the af/shared-guard branch August 31, 2019 03:23
@antonfirsov
Copy link
Member Author

To me it looks like the "Shared project" is a concept predating the new SDK-style msbuild projects, and could be replaced by a simple <Import ... /> nowadays.

We should investigate these practices when we share .props / .targets across our repositories.

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