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

Bug/1061 Cannot mark members as protected in class diagram #1081

Conversation

jgreywolf
Copy link
Contributor

@jgreywolf jgreywolf commented Nov 19, 2019

Using # to indicate protected status of a member or method causing parser error when not used inside class declaration brackets {}. Removed '#' from LABEL regex

Resolves #1061

@klemmchr
Copy link
Member

klemmchr commented Nov 19, 2019

Great, thanks for your contribution. According to our contribution docs, prs go into development. Could you rebase onto it and then change the target branch to it? Thanks!

Your newly added unit test doesn't pass, you might want to look into that.

jgreywolf and others added 2 commits November 19, 2019 12:49
Using # to indicate protected status of a member or method causing parser error when not used inside class declaration brackets {}.  Removed '#' from `LABEL` regex
@jgreywolf jgreywolf force-pushed the Bug1061-CannotMarkMembersAsProtectedInClassDiagram branch from e2f00d0 to 68c2ea3 Compare November 19, 2019 20:50
@jgreywolf jgreywolf changed the base branch from master to develop November 19, 2019 20:52
@jgreywolf
Copy link
Contributor Author

Your newly added unit test doesn't pass, you might want to look into that.

How can I run unit tests locally? Not finding anything in documentation...

@jgreywolf
Copy link
Contributor Author

I fixed the syntax error in the failing test (duh), and now those are all fine. But I see that the GitHub builds failed.. I dont understand why, based on the messages provided though.

@klemmchr
Copy link
Member

I fixed the syntax error in the failing test (duh), and now those are all fine. But I see that the GitHub builds failed.. I dont understand why, based on the messages provided though.

This is not related to your code. We just introduced GitHub builds as a testing phase, for now travis matters.

@klemmchr klemmchr requested a review from knsv November 20, 2019 15:58
@klemmchr klemmchr changed the title Bug1061 cannot mark members as protected in class diagram Bug/1061 Cannot mark members as protected in class diagram Nov 20, 2019
Copy link
Collaborator

@knsv knsv left a comment

Choose a reason for hiding this comment

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

This looks neat! Good with improvements to the class diagrams. There is only one thing I would like to add is a render test as well. A render test really helps making sure things does not break without anyone noticing it.

It should not take more than a few minutes. Look at cypress\integration\rendering\classDiagram.spec.js. Just copy a test and replace the diagram code with code showing your changes .

After this I will merge this. 🎉

@jgreywolf
Copy link
Contributor Author

@knsv Are you saying that you would rather there be a separate test? As opposed to just adding to an existing test which is what I had done :)

Which does make sense

@knsv knsv merged commit 5736d52 into mermaid-js:develop Nov 21, 2019
@knsv
Copy link
Collaborator

knsv commented Nov 21, 2019

Good work with this one!

@jgreywolf jgreywolf deleted the Bug1061-CannotMarkMembersAsProtectedInClassDiagram branch December 11, 2019 18:13
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.

3 participants