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

Add basic PlantUML support (feature request #78) #80

Closed
wants to merge 202 commits into from
Closed

Add basic PlantUML support (feature request #78) #80

wants to merge 202 commits into from

Conversation

studix
Copy link
Contributor

@studix studix commented May 24, 2021

  • ArchUnitNET.Domain.PlantUml:
    new namespace which contains the PlantUML parser code ported from ArchUnit java

  • TypesShould / TypeConditionsDefinition:
    contains new method AdhereToPlantUmlDiagram for PlantUML based dependency checks

ExampleArchUnitTestPuml demonstrates the feature

brandhuf and others added 30 commits May 24, 2021 21:39
Changed LoadAssemblies() Test

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…thods

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…ic methods where not assigned correctly

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…and Interface as input

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…cyInMethodBodyTo() methods to accept more input parameters

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
create matching() method for SliceRules

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…der on .net 3.1

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…n found cycles

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…ies between slices

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…e where counted

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…d Fluent.

fix minor issues

Signed-off-by: Pavel Fischer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Pavel Fischer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Pavel Fischer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
brandhuf and others added 20 commits May 24, 2021 21:41
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…r generated MoveNext() method

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…eated the same as in other methods

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
…d generator methods)

Signed-off-by: Fritz Brandhuber <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Sebastijan Mueller <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Sebastijan Mueller <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Sebastijan Mueller <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Sebastijan Mueller <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Sebastijan Mueller <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: smuel <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: smuel <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
Signed-off-by: Marco Studer <[email protected]>
@fgather
Copy link
Contributor

fgather commented Jun 1, 2021

something bad happened :) The diff contains all Commits since some years, which makes it very hard to review. Do you know how this happened? Looks like you recommitted all the history, only with the original author?

Copy link

@fgathercoupa fgathercoupa 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! The code looks very clean to me and I only have a few remarks :)


public override int GetHashCode()
{
return -1939223833 + EqualityComparer<string>.Default.GetHashCode(_value);

Choose a reason for hiding this comment

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

how did you generate the equals/gethashcode methods? would it be possible to use the convention we have in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Visual Studio 2019 generated that code. I will have a look at other equals methods and use that convention.

Choose a reason for hiding this comment

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

I can also do this. I assume we used Rider/Resharper

[assembly: InternalsVisibleTo("ArchUnitNETTests")]
namespace ArchUnitNET.Domain.PlantUml
{
internal class PlantUmlDiagram

Choose a reason for hiding this comment

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

should all those domain classes be internal? or could some be accessible to be used without the fluent api? Like most other domain classes

Copy link
Contributor Author

@studix studix Jun 2, 2021

Choose a reason for hiding this comment

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

My intention was to encapsulate the whole parser code. This code should not be part of the libraries public API as it is an implementation detail.

Choose a reason for hiding this comment

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

ok, sounds reasonable. However, this somehow implies that this feature won't be able with the non-fluent API. which is fine, but maybe there is a nice way to enable it later.

internal class PlantUmlParser
{
private PlantUmlPatterns _plantUmlPatterns = new PlantUmlPatterns();
public PlantUmlDiagram Parse(string filename)

Choose a reason for hiding this comment

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

we could additionally allow for streams. That could make it easier to test against architecture documentations, but more importantly: we could simplify some test code, which would not need to write files, but the test files could be inserted as an in memory stream

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

return new TestDiagram(path);
}

public ComponentCreator Component(string componentName)

Choose a reason for hiding this comment

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

suggestion: rename to ComponentBuilder? That might improve consistency with other builders in the project

import -left-> catalog : parse products
import --> xml
import --> [Customers]
note top on link #lightgreen: is responsible for translating XML to java classes

Choose a reason for hiding this comment

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

Java? What is this :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy & paste

@studix
Copy link
Contributor Author

studix commented Jun 2, 2021

fgathercoupa> something bad happened :) The diff contains all Commits since some years, which makes it very hard to review. Do you know how this happened? Looks like you recommitted all the history, only with the original author?

I initially forgot to add a signoff message to my commits. In the pull request a rebase command was suggested by github to fix this. Unfortunatly this command also added my signoff message to old commits. I would suggest to reject my pull request. I'm worried about that this will be merged also to the main branch when you accept the pull request. I could take over my changes to a new branch in my fork and create a new pull request. What do you think?

@fgathercoupa
Copy link

fgathercoupa> something bad happened :) The diff contains all Commits since some years, which makes it very hard to review. Do you know how this happened? Looks like you recommitted all the history, only with the original author?

I initially forgot to add a signoff message to my commits. In the pull request a rebase command was suggested by github to fix this. Unfortunatly this command also added my signoff message to old commits. I would suggest to reject my pull request. I'm worried about that this will be merged also to the main branch when you accept the pull request. I could take over my changes to a new branch in my fork and create a new pull request. What do you think?

Hi,
I won't merge it accidentally, but yes we should fix it. Maybe it is possible to cherry-pick your commits into a clean branch? And change the source branch to the new branch?

@studix
Copy link
Contributor Author

studix commented Jun 2, 2021

fgathercoupa> something bad happened :) The diff contains all Commits since some years, which makes it very hard to review. Do you know how this happened? Looks like you recommitted all the history, only with the original author?
I initially forgot to add a signoff message to my commits. In the pull request a rebase command was suggested by github to fix this. Unfortunatly this command also added my signoff message to old commits. I would suggest to reject my pull request. I'm worried about that this will be merged also to the main branch when you accept the pull request. I could take over my changes to a new branch in my fork and create a new pull request. What do you think?

Hi,
I won't merge it accidentally, but yes we should fix it. Maybe it is possible to cherry-pick your commits into a clean branch? And change the source branch to the new branch?

What do you mean with "change the source branch to the new branch"?

@studix
Copy link
Contributor Author

studix commented Jun 8, 2021

I would have a clean new branch ready on my fork. It also contains the changes from the review. Shall we delete this pull request and I will create a new one?

@fgather
Copy link
Contributor

fgather commented Jun 8, 2021 via email

@studix
Copy link
Contributor Author

studix commented Jun 13, 2021

Pull request replaced by pull request #86

@studix studix closed this Jun 13, 2021
@studix studix deleted the puml branch June 13, 2021 06:18
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.

8 participants