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 helper methods to create mappings #38

Merged
merged 6 commits into from
Mar 28, 2017
Merged

Conversation

muuki88
Copy link
Contributor

@muuki88 muuki88 commented Mar 2, 2017

  • directory: create mappings from a baseDirectory including the baseDirectory
  • contentOf: create mappings from a baseDirectory excluding the baseDirectory

I put this on the Path object. Not sure if this is a good place.
The test aren't property based. I probably need to add some corner cases like

  • empty baseDirectory
  • if baseDirectory is a file
  • non-existing baseDirectory

- directory: create mappings from a baseDirectory including the baseDirectory
- contentOf: create mappings from a baseDirectory excluding the baseDirectory
@eed3si9n eed3si9n added the ready label Mar 2, 2017
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

I've a few comments, but also I think this should probably live with its brothers in Mapper (PathMapper.scala).

Thank you for taking the time to PR!

* @example Add a static directory "extra" and re-map the destination to a different path
* {{{
* mappings in Universal ++= contentOf(baseDirectory.value / "extra").map {
* case (src, destination => src -> s"new/path/$destination"
Copy link
Member

Choose a reason for hiding this comment

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

this example doesn't compile (missing )) and is missing an explanation at the end.

* File(extras/file1) -> "extras/file1"
* File(extras/file2) -> "extras/"file2"
* ...
* }}}
Copy link
Member

Choose a reason for hiding this comment

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

This example seems to be wrong. I'm getting:

scala> for ((file, path) <- Path directory (file("base") / "extra")) println(s"""$file -> "$path"""")
base/extra -> "extra"
base/extra/file1 -> "extra/file1"
base/extra/file2 -> "extra/file2"

* File(extras/file1) -> "file1"
* File(extras/file2) -> "file2"
* ...
* }}}
Copy link
Member

Choose a reason for hiding this comment

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

This example seems to be wrong. I'm getting:

scala> for ((file, path) <- Path contentOf (file("base") / "extra")) println(s"""$file -> "$path"""")
base/extra/file1 -> "file1"
base/extra/file2 -> "file2"

@eed3si9n
Copy link
Member

@dwijnand What's the status on this PR?

* File($baseDirectory/extras/file1) -> "extras/file1"
* File($baseDirectory/extras/file2) -> "extras/"file2"
* ...
* }}}
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this still missing $baseDirectory/extra -> "extra"?

*
* {{{
* File($baseDirectory/extras/file1) -> "extras/file1"
* File($baseDirectory/extras/file2) -> "extras/"file2"
Copy link
Member

Choose a reason for hiding this comment

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

there's an extra "

* @example Add a static directory "extra" and re-map the destination to a different path
* {{{
* mappings ++= contentOf(baseDirectory.value / "extra").map {
* case (src, destination => src -> s"new/path/$destination"
Copy link
Member

Choose a reason for hiding this comment

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

there's still a missing ) here

@dwijnand
Copy link
Member

I just submitted the review that I had pending from 2 weeks ago..

Haven't reviewed the latest 2 commits.

@muuki88 muuki88 force-pushed the add-mapping-helpers branch from dbe6eab to 2b184f0 Compare March 24, 2017 16:57
@muuki88
Copy link
Contributor Author

muuki88 commented Mar 24, 2017

Sorry for the long standing PR. Finally got time to clean this up

@dwijnand dwijnand self-assigned this Mar 27, 2017
Copy link
Member

@dwijnand dwijnand left a comment

Choose a reason for hiding this comment

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

Thanks Muki!

@dwijnand dwijnand merged commit 47725b5 into sbt:1.0 Mar 28, 2017
@dwijnand dwijnand removed the ready label Mar 28, 2017
@muuki88 muuki88 deleted the add-mapping-helpers branch March 28, 2017 11:17
@dwijnand dwijnand removed their assignment Apr 17, 2017
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.

3 participants