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

Scala3doc/community build #10522

Merged
merged 7 commits into from
Dec 3, 2020

Conversation

romanowski
Copy link
Contributor

No description provided.

@romanowski romanowski force-pushed the scala3doc/community-build branch 4 times, most recently from 462c02c to 83af211 Compare December 1, 2020 19:50
@romanowski romanowski marked this pull request as ready for review December 1, 2020 19:50
Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

community-build/src/scala/dotty/communitybuild/Main.scala Outdated Show resolved Hide resolved
community-build/src/scala/dotty/communitybuild/Main.scala Outdated Show resolved Hide resolved
@smarter
Copy link
Member

smarter commented Dec 2, 2020

I'm not super happy about the state of this because it adds quite a lot of complexity to the community build which is already too complex for us to manage efficiently:

  • projectMap just duplicates information, surely there's a better way to do that that doesn't lead to bugs when we forget to add/update entries
  • forceDoc looks like a hack, instead of doing that if you need to enable docs in some projects, we should edit these submodules to enable docs there (and make a PR upstream)
  • When we publishLocal a project we already run documentation on it, so is there really a need for separately listing doc tasks in projects?

Anyway, @anatoliykmetyuk is the maintainer of the community build so I'll let him decide whether this is acceptable as is.

@abgruszecki
Copy link
Contributor

abgruszecki commented Dec 2, 2020

Reg. @smarter's comments:

  1. I don't think we can easily avoid having projectMap. We cannot construct it in constructor of Project, since projects need to be lazy vals, as they reference one another. We could consider defining it with a macro. Realistically, I don't think the duplication is an actual problem, we will quickly get an error if a project is missing from the map.

    EDIT: of course, if we were writing in a Lisp, we could trivially write a macro for defining projects. Maybe someday we will be able to do the same in Scala.

  2. As far as hacks go, forceDoc is just fine. We can clean it up as we go along.

  3. See above, unless we clean up forceDoc we can't just depend on publishLocal.

@romanowski
Copy link
Contributor Author

romanowski commented Dec 2, 2020

forceDoc looks like a hack, instead of doing that if you need to enable docs in some projects, we should edit these submodules to enable docs there (and make a PR upstream)

Actually I think that maintaning forceDoc would be much easier then maintaning changes in N seperate projects since it will still works if projcets migrate to scala3doc (or remove hacks to disable docs)

When we publishLocal a project we already run documentation on it, so is there really a need for separately listing doc tasks in projects?

The problem is that for now we need to force useScala3doc. We will be able to get rid of both doc task and forceDoc once scala3doc will be a default doctool and will be used within our community build.

of course, if we were writing in a Lisp, we could trivially write a macro for defining projects. Maybe someday we will be able to do the same in Scala.

@abgruszecki could someday be today? :)

@smarter
Copy link
Member

smarter commented Dec 2, 2020

Actually I think that maintaning forceDoc would be much easier then maintaning changes in N seperate projects since it will still works if projcets migrate to scala3doc (or remove hacks to disable docs)

We don't need to maintain changes in these projects if we make PR upstream.

The problem is that for now we need to force useScala3doc.

I suggest making that the default now in sbt-dotty, we don't have a lot of time left to make the switch so the sooner the better.

val projectsTree = Term.of(from)
val symbols = TypeTree.of[V].symbol.members.filter(isProjectField)
val selects = symbols.map(Select(projectsTree, _).asExprOf[T])
'{ println(${Expr(retType.show)}); ${Varargs(selects)} }
Copy link
Contributor

Choose a reason for hiding this comment

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

Macro looks ok, but the println probably shouldn't be here, no?

@abgruszecki
Copy link
Contributor

The problem is that for now we need to force useScala3doc.

I suggest making that the default now in sbt-dotty, we don't have a lot of time left to make the switch so the sooner the better.

Yes, I think by M3 we should make useScala3doc := true by default. First though, we should probably ensure we can run Scala3doc on all projects in the community build. Looking at the comments in projects.scala, I may already have fixed some of the errors mentioned there.

Can we merge this PR first, so that we can cooperate on that?

Copy link
Contributor

@anatoliykmetyuk anatoliykmetyuk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@griggt griggt left a comment

Choose a reason for hiding this comment

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

The rebase error removing the scalacheck dependency on munit needs fixing.

@romanowski
Copy link
Contributor Author

The rebase error removing the scalacheck dependency on munit needs fixing.

Fixed.

Copy link
Contributor

@griggt griggt left a comment

Choose a reason for hiding this comment

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

I'm not a fan of the overall hacky nature (hopefully most of that is temporary).

Approving only to unblock my change request, thanks for fixing the rebase errors.

@romanowski romanowski merged commit bbdbdf2 into scala:master Dec 3, 2020
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.

5 participants