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

HTML Comment Blocks #24

Merged
merged 13 commits into from
Jul 1, 2019
Merged

HTML Comment Blocks #24

merged 13 commits into from
Jul 1, 2019

Conversation

jonaskordt
Copy link
Contributor

This is ready and can be reviewed even though the tests in 5.0 are failing as we plan to no longer support 5.0. No reason why you couldn't review it yet though.

Obviously merging will have to wait for the removal of 5.0 support.

jonaskordt and others added 2 commits June 24, 2019 15:56
Co-authored-by: Lara Pfennigschmidt <lara.pfennigschmidt@student.hpi.de>
Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
-makes it consistent with the BlockTextStyler


Co-authored-by: Lara Pfennigschmidt <lara.pfennigschmidt@student.hpi.de>
@jonaskordt jonaskordt added the Feature New functionality label Jun 25, 2019
@jonaskordt jonaskordt changed the title HTML Comment Blocks WIP: HTML Comment Blocks Jun 25, 2019
Copy link
Contributor

@felix-gohla felix-gohla left a comment

Choose a reason for hiding this comment

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

Perfect. Works great.

Copy link
Contributor

@lpfennigschmidt lpfennigschmidt left a comment

Choose a reason for hiding this comment

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

:D

Copy link
Contributor

@kiragrammel kiragrammel left a comment

Choose a reason for hiding this comment

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

Some class comments are missing e.g. for all MarkdownBlock classes. I don't know if we need class comments for the test classes. 🤔

Copy link
Contributor

@lpfennigschmidt lpfennigschmidt left a comment

Choose a reason for hiding this comment

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

And yes, we should write class comments ^^

Copy link
Contributor

@pixunil pixunil left a comment

Choose a reason for hiding this comment

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

The code reads like a charm, the only puzzling moment is - like always - matches. I hope that my interpretation of the spec is correct, and that you can simplify it.


| commentBlock |
commentBlock := MarkdownCommentBlock newFrom: '<!-- MiniMetro', Character cr.
commentBlock append: 'is great --> more Text'.
Copy link
Contributor

Choose a reason for hiding this comment

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

	commentBlock := MarkdownCommentBlock newFrom: '<!-- Comment with', Character cr.
	commentBlock append: 'multiple lines --> Text after end tag'.

Applies to other isClosed and commentEnd tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Meh. I think it's pretty clear like it is...

tests - matching
testMismatchOverlappingOpeningAndClosingTag

self deny: (MarkdownCommentBlock matches: '<!-->').
Copy link
Contributor

Choose a reason for hiding this comment

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

after reading the spec, I think that the rules

  1. Up to 3 indentation
  2. Begins with <!--

are sufficient. It differs for inline comments though, where <!--> and <!---> and <!-- -- --> are forbidden.
So I think these tests are wrong and you can remove some lines in matches:. But maybe I just misread the spec...

Copy link
Contributor Author

@jonaskordt jonaskordt Jun 28, 2019

Choose a reason for hiding this comment

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

Hm seems like you are right. I think it's confusing that they have different rules abut trying it with https://spec.commonmark.org/dingus/ gives the result you described.

(I changed it already but didn't resolve it to avoid confusion)

jonaskordt and others added 3 commits June 29, 2019 00:21
-corrects `MarkdonwCommentBlock>>matches:` to comply with spec
-improves some coding standards
-adds accessort for `openingTag` and `closingTag` on instance side of `MarkdownCommentBlock`
@jonaskordt jonaskordt requested a review from pixunil June 29, 2019 14:53
Copy link
Contributor

@pixunil pixunil left a comment

Choose a reason for hiding this comment

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

No big suggestions left. But the discussion about the test strings remains...

@jonaskordt jonaskordt changed the title WIP: HTML Comment Blocks HTML Comment Blocks Jul 1, 2019
@jonaskordt jonaskordt merged commit 5993c26 into master Jul 1, 2019
@jonaskordt jonaskordt deleted the feature/comment-blocks branch July 1, 2019 12:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants