-
Notifications
You must be signed in to change notification settings - Fork 1
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
Simplifies TextAttributes #35
Conversation
- deletes MarkdownTextAttributeBold, MarkdownTextAttributeItalic - adds convenience initializer bold and italic - MarkdownTextAttribute only has one squeak text attribute Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some ideas come late, but I hope not too late ^^'
packages/MarkdownEditor-Core.package/MarkdownAttribute.class/properties.json
Outdated
Show resolved
Hide resolved
packages/MarkdownEditor-Core.package/MarkdownAttribute.class/README.md
Outdated
Show resolved
Hide resolved
...ge/MarkdownInlineTextStylerTest.class/instance/testInterpretTokensCutDelimiterClosesTwice.st
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like it, but I also agree with the others about renaming to MarkdownEmphasis and adding a utility method for testing :D
packages/MarkdownEditor-Core.package/MarkdownAttribute.class/properties.json
Outdated
Show resolved
Hide resolved
Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
- Emphases comparing Utilitymethod - MarkdownEmphasis bold and italic is now strongFrom: to: and from: to: - removed some code smell Co-authored-by: Felix Gohla <felix.gohla@student.hpi.de>
…l :o Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I don't fear those inline tests anymore ^^
Shouldn't attributes
now become emphases
?
Regarding the assert:areEmphasesIn:
, I find it strange to supply hash
. Supplying =
is not ideal, but is relatively trivial, whereas hash
looks really like having testing logic where it does not belong
...r-Core.package/MarkdownEmphasisDelimiter.class/instance/attributeFrom.to.delimiterLength..st
Outdated
Show resolved
Hide resolved
...MarkdownEditor-Core.package/MarkdownEmphasisDelimiter.class/instance/attributeStartingAt..st
Outdated
Show resolved
Hide resolved
...e/MarkdownEmphasisDelimiterTest.class/instance/testDifferentDelimitersCreateBoldAttribute.st
Outdated
Show resolved
Hide resolved
...e/MarkdownEmphasisDelimiterTest.class/instance/testDifferentDelimitersCreateBoldAttribute.st
Outdated
Show resolved
Hide resolved
...ackage/MarkdownEmphasisDelimiterTest.class/instance/testSameDelimitersCreateBoldAttribute.st
Outdated
Show resolved
Hide resolved
ifFound: [:foundOpener | | ||
self deny: (openCloser = foundOpener). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this deny
redundant? We know that opener
and openCloser
are two variables, and when we assert that the foundOpener
is opener
, it can't be openCloser
by transitivity, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just left it in. In my point of view, this "redundancy" makes the test easier to understand. From a logical standpoint, you're absolutely right. But pointing out, that it is not the openCloser
but the opener
adds some clarity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, in my case it was the opposite, because I have both of a positive and a negative assert and we actually want to test equality. As a reader, I need to decide whether the deny or the assert is the important part. Thus, it doesn't make it clearer for me, but confuses me...
...or-Tests.package/MarkdownInlineTextStylerTest.class/instance/testInterpretTokensAsItalics.st
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Jakob mostly.
The main problem I see is having the MarkdownEmphasis>>=
and MarkdownEmphasis>>hash
in production code, where it doesn't belong.
The rest is fine with me.
- The emphasis comparison (>>= and >>hash) is now part of the *MarkdownEditor-Tests package and therefore it will only be distributed when installing the test package. Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do as much as you want to do of the open issues, but I think the core idea of this refactoring is implemented :D
Note: While it may not be the most ideal convention, we try to use ItalicEmphasis and StrongEmphasis in the tests to differentiate between "normal" *emphasis* and **strong emphasis** Co-authored-by: Kira Grammel <kira.grammel@student.hpi.de>
Fixes #18.
One comment: In the tests we e.g. check against
MarkdownAttribute bold textAttribute
. Should we change that toTextEmphasis bold
?Co-authored-by: Kira Grammel kira.grammel@student.hpi.de