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 ScastieModifier #59

Merged
merged 2 commits into from
Aug 28, 2018
Merged

Add ScastieModifier #59

merged 2 commits into from
Aug 28, 2018

Conversation

gabro
Copy link
Member

@gabro gabro commented Aug 27, 2018


class ScastieModifierSuite extends BaseMarkdownSuite {

private val debugClassSuffix = "test"
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed because I generate a random UUID for suffixing the CSS class of each snippet so diff wouldn't work.

Copy link
Member

@olafurpg olafurpg 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 awesome! Thank you @gabro a few minor comments, otherwise looks good

* ```
* }}}
* ====Note====
* The empty line in the block is relevant (md parser chokes otherwise)
Copy link
Member

Choose a reason for hiding this comment

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

Copy-paste?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, on purpose.

The warning applies to both cases and I suspect people that jump through sections won't catch it unless repeated.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,82 @@
package mdoc.modifier
Copy link
Member

Choose a reason for hiding this comment

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

let's call this scastie.mods instead

Copy link
Member Author

Choose a reason for hiding this comment

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

mdoc.mods? Anyway I'm sold on the plural (following https://softwareengineering.stackexchange.com/questions/75919/should-package-names-be-singular-or-plural/327653), but I'm not sold on the abbreviation, because you end up with Modifier for the class names, but mod for the package.

I would rather use Mod for the classes (ScastieMod), or use modifiers for the package name.

Copy link
Member

Choose a reason for hiding this comment

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

I may eventually go with Mod instead of Modifier since it's quite long. I renamed the package to mdoc.modifiers for now since I agree with the stack exchange answer you linked about plural is nice when all members of the package have the same type.

@@ -61,4 +61,23 @@ abstract class BaseMarkdownSuite extends org.scalatest.FunSuite with DiffAsserti
assertNoDiffOrPrintExpected(obtained, expected)
}
}

def checkWithSettings(
Copy link
Member

Choose a reason for hiding this comment

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

I think this is unnecessary if you override val settings. Alternatively, can we reuse the check above by adding a paramter settings: Settings = this.setting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Overriding the settings doesn't allow to assign different settings to different tests.

Passing settings to check doesn't work because context has already been initialized at that point.

I can refactor the BaseMarkdownSuite to be more stateless, but that's a bigger change.

Copy link
Member

Choose a reason for hiding this comment

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

I would have created a ScastieDarkSuite to work around the evil state 😜

I refactored BaseMarkdownSuite to be state-less so that it's possible to use different settings in the same suite, just for you 😘

@MasseGuillaume
Copy link

Really cool! Can you add a note in the doc to about preferring the embedding with an ID. This way you don't need to re-run the snippet.

@jvican
Copy link
Collaborator

jvican commented Aug 27, 2018

This looks really cool.

@gabro
Copy link
Member Author

gabro commented Aug 27, 2018

@MasseGuillaume it's already there, right below the inline snippet example. Or did you mean something different?

* ```
* }}}
* ====Note====
* This will be slower to run than embedding a snippet, since it's not cached
Copy link
Member Author

Choose a reason for hiding this comment

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

@MasseGuillaume here ⬆️

- make BaseMarkdownSuite stateless to enable different settings in the
  same suite
- s/mdoc.modifier/mdoc.modifiers/
Copy link
Member

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍 Thank you @gabro ! I pushed a few changes to address my comments.

@@ -0,0 +1,82 @@
package mdoc.modifier
Copy link
Member

Choose a reason for hiding this comment

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

I may eventually go with Mod instead of Modifier since it's quite long. I renamed the package to mdoc.modifiers for now since I agree with the stack exchange answer you linked about plural is nice when all members of the package have the same type.

* ```
* }}}
* ====Note====
* The empty line in the block is relevant (md parser chokes otherwise)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -61,4 +61,23 @@ abstract class BaseMarkdownSuite extends org.scalatest.FunSuite with DiffAsserti
assertNoDiffOrPrintExpected(obtained, expected)
}
}

def checkWithSettings(
Copy link
Member

Choose a reason for hiding this comment

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

I would have created a ScastieDarkSuite to work around the evil state 😜

I refactored BaseMarkdownSuite to be state-less so that it's possible to use different settings in the same suite, just for you 😘

@olafurpg
Copy link
Member

GitHub is funky so it's no picking up the push, guess I should hit bed then 😏

@olafurpg olafurpg merged commit 15a5438 into scalameta:master Aug 28, 2018
@olafurpg
Copy link
Member

Forgot, can you please document this modifier in docs/readme.md? Docs are generated with mdoc via sbt docs/run

@gabro
Copy link
Member Author

gabro commented Aug 28, 2018

It's website/run I presume

@gabro gabro deleted the scastie-modifier branch August 28, 2018 08:20
@olafurpg
Copy link
Member

whoops should rename that module to docs for consistency!

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.

None yet

4 participants