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

Type safety at micrositeCompilingDocsTool Setting #297

Merged
merged 2 commits into from Jan 15, 2019

Conversation

Projects
None yet
3 participants
@juanpedromoreno
Copy link
Contributor

juanpedromoreno commented Jan 14, 2019

This PR brings a new ADT that users can use at their convenience, with two possibilities: tut or mdoc.

@@ -28,8 +28,8 @@ import sbt.Keys._
import sbt._
import sbt.complete.DefaultParsers.OptNotSpace
import sbtorgpolicies.github.GitHubOps
import tut.TutPlugin.autoImport._
import mdoc.MdocPlugin.autoImport._
import tut.TutPlugin.autoImport.{tut => tutTask, _}

This comment has been minimized.

@fedefernandez

fedefernandez Jan 14, 2019

Contributor

Do we need this TutPlugin autoImport and not the one for MdocImport?

This comment has been minimized.

@rafaparadela

rafaparadela Jan 14, 2019

Member
import tut.TutPlugin.autoImport.{tut => tutTask, _}
import mdoc.MdocPlugin.autoImport.{mdoc => mdocTask}
@@ -43,6 +43,10 @@ trait MicrositeKeys {
final case object GHPagesPlugin extends PushWith("ghPagesPlugin")
final case object GitHub4s extends PushWith("github4s")

sealed abstract class CompilingDocsTool extends Product with Serializable
final case object tut extends CompilingDocsTool
final case object mdoc extends CompilingDocsTool

This comment has been minimized.

@fedefernandez

fedefernandez Jan 14, 2019

Contributor

This is a bit redundant, right? I guess these can be just case object

@juanpedromoreno

This comment has been minimized.

Copy link
Contributor Author

juanpedromoreno commented Jan 14, 2019

This is going to fail given mdoc and tut are reserved words when they are auto-triggered (AutoPlugin) where they are used :(

[error] it is imported twice in the same scope by
[error] import _root_.mdoc.MdocPlugin.autoImport._
[error] and import _root_.microsites.MicrositesPlugin.autoImport._

Therefore, we might have two options:

  • Renaming tut and mdoc case objects by something else (WithTut, WithMdoc? 🤔 ).
  • Closing this PR and leaving settings as it is (String values).

Thoughts?

@fedefernandez

This comment has been minimized.

Copy link
Contributor

fedefernandez commented Jan 14, 2019

Will Tut and MDoc have the same problem?

@rafaparadela
Copy link
Member

rafaparadela left a comment

LGTM

@rafaparadela

This comment has been minimized.

Copy link
Member

rafaparadela commented Jan 14, 2019

If, as Fede suggested, Tut and Mdoc work fine, seem to be the best choice. Otherwise WithTut and WithMdoc are not too bad either.

@juanpedromoreno

This comment has been minimized.

Copy link
Contributor Author

juanpedromoreno commented Jan 15, 2019

Tut is also a reserved word in the tut side. I'll use WithTut, WithMdoc if there are no inconveniences.

@juanpedromoreno juanpedromoreno merged commit 62b1cfb into master Jan 15, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@juanpedromoreno juanpedromoreno deleted the jp-adt-mdoc-tut branch Jan 15, 2019

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