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.

Sign up for GitHub

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

Jump to bottom

FS-165 Module looks from Root #239

Closed
wants to merge 2 commits into from
Closed

FS-165 Module looks from Root #239

wants to merge 2 commits into from

Conversation

diesalbla
Copy link
Contributor

This PR addresses ticket #165. It modifies the @module macro, so that now it looks for the Op types of the algebra using the `root prefix.

One side effect of this changes is that it restricts the @module macro to use @free or @module traits with a defined package, which excludes those defined in a Scala console session, or (for extension) in a tut book. For instance, pasting this to the console:

import freestyle._
@free trait Foo { def x: FS[Int] }
@module trait Bar[F] {val y: Foo[F] }

would no longer work. For this reason we have to adapt the many tut:book items in the docs, adding a few Scala files.

@diesalbla
FS-237 Repair Macro Error
e3af758 
We fix the bug reported with the module macro. Now the references
to algebras types start with the _root_ base.
@diesalbla
FS-237 Adjust Docs to new Module Macro.
3711a34 
As an effect of the recent changes in `@module`, now it is not
possible to have in a `tut` session an `@free` algebra and a `@module`
that uses it. For this reason, we need to copy those algebras (or
modules) to auxiliary scala files, and import them in the `tut` books.
@codecov-io
Copy link

codecov-io commented Apr 20, 2017 •
edited
Loading

Codecov Report

Merging #239 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #239   +/-   ##
=======================================
  Coverage   75.66%   75.66%           
=======================================
  Files          35       35           
  Lines         300      300           
  Branches        2        2           
=======================================
  Hits          227      227           
  Misses         73       73

peterneyens
peterneyens reviewed Apr 20, 2017
View reviewed changes
docs/src/main/tut/docs/patterns/logging/README.md
@@ -17,7 +17,7 @@ libraryDependencies += "com.47deg" %% "freestyle-logging" % "0.1.0"

### Operations

The set of abstract operations of the `Logging` algebra are specified as follows:
The set of abstract operations of the [`Logging` algebra](../../../../../../../freestyle-logging/shared/src/main/scala/logging.scala) are specified as follows:
Copy link
Member

Choose a reason for hiding this comment

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

What happens if you click on this link on the microsite?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Run locally, it fails. It is intended to be use in github, as a link to the source code.

Copy link
Member

@peterneyens peterneyens Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

If we used an absolute link (eg https://github.com/47deg/freestyle/blob/master/freestyle-logging/shared/src/main/scala/logging.scala) it would work on the website as well, right?

@peterneyens
Copy link
Member

Hmm, not being able to create a module in the REPL, tut (and I expect scastie), seems kind of a hard price to pay?

peterneyens
peterneyens reviewed Apr 20, 2017
View reviewed changes
docs/src/main/tut/docs/README.md
@@ -80,9 +83,11 @@ Abstract definitions it's all it takes to start building programs that support s
The example below combines both algebras to produce a more complex program

```tut:book
import cats.syntax.cartesian._
Copy link
Member

Choose a reason for hiding this comment

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

The cats documentation choose to always use import cats.implicits._ instead of the piecemeal imports as it is more beginner friendly, should we do the same?

Copy link
Contributor Author

@diesalbla diesalbla Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

IMHO, it is more beginner-friendly to narrow down, as much as possible, what is it that the code snippet is using from this big cats library. Not everybody may know all of cats.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that was my initial point of view as well, but it is even more difficult to figure out where exactly which syntax is coming from before you know the relation between all the concepts Cats provides, see typelevel/cats#1026.

@diesalbla
Copy link
Contributor Author

You can create a module, and the changes in the tut:book are still creating modules. What it can not do is to use the @module or @free traits defined in the same console, tut, or scastie.

@peterneyens
Copy link
Member

Creating a @module or @free traits in the REPL without being able to use them, is not very useful, so my wording may have been wrong, but this still means that it becomes impossible to use freestyle in the REPL which to me seems sort of a big deal.

@raulraja raulraja mentioned this pull request Apr 20, 2017
Closed
raulraja
raulraja requested changes Apr 20, 2017
View reviewed changes
Copy link
Contributor

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

We are putting this on hold because fixing the FQN names creates the side effect of @module not working in REPL environments. See comments on #165 for details

@diesalbla diesalbla closed this Nov 30, 2018
@diesalbla
Copy link
Contributor Author

This has fallen far behind, and the problem may not be a major one.

@raulraja
Copy link
Contributor

raulraja commented Dec 2, 2018

@diesalbla feel free to reopen if you consider this can be fixed now

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

@peterneyens peterneyens peterneyens left review comments

@raulraja raulraja raulraja requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@diesalbla @codecov-io @peterneyens @raulraja