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

Codemirror support #17

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ftomassetti
Contributor

ftomassetti commented Apr 5, 2015

(See Issue #9 for context)

This is an initial implementation of the CodeMIrror mode specific for PlantUML.
I started building Syntax Highlighting for Activity and Class diagrams.
At this stage I could use some feedback to understand if I am going in the right direction. If that is the case we could work also on:

  • supporting the remaining types of diagrams
  • polishing the code
  • add other features like auto-completion

What do you think?

@maximesinclair

This comment has been minimized.

Show comment
Hide comment
@maximesinclair

maximesinclair Apr 12, 2015

Contributor

Sorry for the delay but I'm very busy at the moment.
First question, why did you create another script section ? Some difficulties with the one already present in the head ?
Concerning the name of the js mode file, we'll either keep this name but in a "mode" directory or we'll need to rename it to something like "plantumlmode.js". What do you think ?
I have to admit that I don't take the time to test it but I will do it soon. The code seems to be clear. Sorry for my late feedback.

Contributor

maximesinclair commented Apr 12, 2015

Sorry for the delay but I'm very busy at the moment.
First question, why did you create another script section ? Some difficulties with the one already present in the head ?
Concerning the name of the js mode file, we'll either keep this name but in a "mode" directory or we'll need to rename it to something like "plantumlmode.js". What do you think ?
I have to admit that I don't take the time to test it but I will do it soon. The code seems to be clear. Sorry for my late feedback.

@ftomassetti

This comment has been minimized.

Show comment
Hide comment
@ftomassetti

ftomassetti Apr 12, 2015

Contributor

No problem @maximesinclair we are all busy :)
What do you mean by another? There was already a CodeMirror mode for PlantUML? Did I miss it?

Contributor

ftomassetti commented Apr 12, 2015

No problem @maximesinclair we are all busy :)
What do you mean by another? There was already a CodeMirror mode for PlantUML? Did I miss it?

@maximesinclair

This comment has been minimized.

Show comment
Hide comment
@maximesinclair

maximesinclair Apr 12, 2015

Contributor

It's seems that the insertion of html tags in comment is not really supported. Please reread it now, I deleted the brackets around the "script" word.

Contributor

maximesinclair commented Apr 12, 2015

It's seems that the insertion of html tags in comment is not really supported. Please reread it now, I deleted the brackets around the "script" word.

@ftomassetti

This comment has been minimized.

Show comment
Hide comment
@ftomassetti

ftomassetti Apr 12, 2015

Contributor
  • ok, I will move it to a mode directory, uncommenting the previous script statement
  • about testing: I have tests for this mode here https://github.com/ftomassetti/codemirror_plantuml. You have just to clone the project and open test/index.html. I could include the tests in this pull request if you want
Contributor

ftomassetti commented Apr 12, 2015

  • ok, I will move it to a mode directory, uncommenting the previous script statement
  • about testing: I have tests for this mode here https://github.com/ftomassetti/codemirror_plantuml. You have just to clone the project and open test/index.html. I could include the tests in this pull request if you want
@ftomassetti

This comment has been minimized.

Show comment
Hide comment
@ftomassetti

ftomassetti Apr 25, 2015

Contributor

Any comment or suggestion?

Contributor

ftomassetti commented Apr 25, 2015

Any comment or suggestion?

@maximesinclair

This comment has been minimized.

Show comment
Hide comment
@maximesinclair

maximesinclair Apr 25, 2015

Contributor

Imho, the test suite is a very interesting source that must be included. Maybe it will be a little difficult to run it as part of the maven build but it's already useful as a manual test.
Concerning the mode definition file, I think it deserves more comments and a graphic view of the state machine could be useful. The state names could be normalized (sometimes in mixed case, sometimes lowercase words separated by spaces).
But I can confirm that you are on the right direction and it's already a good start.
My main question concerning a potential future difficulty is : how to split the file into different parts (one by type of diagram, for example) to avoid the too large file effect ? But I suppose that others already found different solutions.

Contributor

maximesinclair commented Apr 25, 2015

Imho, the test suite is a very interesting source that must be included. Maybe it will be a little difficult to run it as part of the maven build but it's already useful as a manual test.
Concerning the mode definition file, I think it deserves more comments and a graphic view of the state machine could be useful. The state names could be normalized (sometimes in mixed case, sometimes lowercase words separated by spaces).
But I can confirm that you are on the right direction and it's already a good start.
My main question concerning a potential future difficulty is : how to split the file into different parts (one by type of diagram, for example) to avoid the too large file effect ? But I suppose that others already found different solutions.

@ftomassetti

This comment has been minimized.

Show comment
Hide comment
@ftomassetti

ftomassetti May 9, 2015

Contributor

About the tests: they need CodeMirror to be run, how we should use them? I think CodeMirror is not present among the src, you probably retrieve it as a dependency, right? What do you think is the best way to retrieve from tests? I was thinking the mode tests should not require a running application.

We could split the code depending on the type of diagrams, maybe with some core shared among several types. As I familiarize more with CodeMirror I will try to build more abstractions to make the code more concise and readable

Contributor

ftomassetti commented May 9, 2015

About the tests: they need CodeMirror to be run, how we should use them? I think CodeMirror is not present among the src, you probably retrieve it as a dependency, right? What do you think is the best way to retrieve from tests? I was thinking the mode tests should not require a running application.

We could split the code depending on the type of diagrams, maybe with some core shared among several types. As I familiarize more with CodeMirror I will try to build more abstractions to make the code more concise and readable

@maximesinclair

This comment has been minimized.

Show comment
Hide comment
@maximesinclair

maximesinclair May 9, 2015

Contributor

I confirm that CodeMirror is retrieved as a maven dependency using webjars (see http://www.webjars.org/). It would be great if the mode tests can be run (and invoked) as a unit test during the maven build. In this maven stage, the CodeMirror can be already retrieved. And for sure, it is not necessary to have a running application to test the CodeMirror mode. I totally agree with that.

About the structure of the code, the most important is readability. Splitting can be a good way to get it, but not the only one. It was just an idea. Eventually, it would not be so necessary. We will see.

Contributor

maximesinclair commented May 9, 2015

I confirm that CodeMirror is retrieved as a maven dependency using webjars (see http://www.webjars.org/). It would be great if the mode tests can be run (and invoked) as a unit test during the maven build. In this maven stage, the CodeMirror can be already retrieved. And for sure, it is not necessary to have a running application to test the CodeMirror mode. I totally agree with that.

About the structure of the code, the most important is readability. Splitting can be a good way to get it, but not the only one. It was just an idea. Eventually, it would not be so necessary. We will see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment