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

Automatically add package declaration when creating a file #862

Merged
merged 2 commits into from Aug 13, 2019

Conversation

@tdudzik
Copy link

commented Aug 9, 2019

Implements #860

@tgodzik tgodzik requested a review from marek1840 Aug 9, 2019
@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

We were working a bit in VirtusLab on these smaller issues, this is an initial PR adding the support for auto-adding packages. What would be also nice is automatically rename packages when moving around and also autocompletion on package.

We use here workspace apply, which needs to be done after a file is open. It gives us the ability to also do stuff like CTRL+Z on the file to remove the edit, which I think is pretty nice. That's why we have an additional ConcurrentSet to keep recently created files. We also check if they were empty and only then add the package declaration.

@tgodzik tgodzik changed the title Implement package auto adding Automatically add package declaration when creating a file Aug 9, 2019
@tgodzik tgodzik requested a review from gabro Aug 9, 2019
Copy link
Collaborator

left a comment

Added a few comments, other than that you need to rebase on master - the code is no longer compiling since there was a change in one of the method's names.

Copy link
Member

left a comment

LGTM! I'm glad to see this landing in Metals proper :)

Aside from the minor comments I've left, have you tested that the edit is only applied for Scala files? E.g. what if I create a Java file or any other type of file? Does it add a package line there too?

Maybe we can add a negative test for that

@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

@gabro It's currently like this:

package com.foo.bar
<CURSOR>

adding another newline is a good idea, let's do that.

We should for sure add a negative test for Java files, but it works currently only for Scala files, so I think we are good.

Copy link
Collaborator

left a comment

Functionality-wise this looks good. I have left some not-that important naming and style suggestions.

@tdudzik tdudzik force-pushed the tdudzik:auto_add_package branch 3 times, most recently from db4cbce to 220d057 Aug 12, 2019
Copy link
Collaborator

left a comment

Just last few nitpicks.

Copy link
Collaborator

left a comment

One suggestion, other than that looks great.

@gabro
gabro approved these changes Aug 12, 2019
@tdudzik tdudzik force-pushed the tdudzik:auto_add_package branch 2 times, most recently from 6d586d2 to 0e055ef Aug 13, 2019
PackageProvider was introduced and can be used to implement autocompletion and package rename after file move.

The set of created files was introduced because we cannot apply the workspace edit before the file is open.
@tdudzik tdudzik force-pushed the tdudzik:auto_add_package branch from 0e055ef to 63a39f1 Aug 13, 2019
@tdudzik

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

@gabro @marek1840 Please take a look one more time. We've decided to get rid of createdFiles set, as it was causing another race condition (especially in the tests) - when didOpen notification was sent before FileWatcher noticed that a new file was created, package name wasn't added.

Also, the package is not added when the file is named package.scala because it should be different than in case of normal scala file. It should be handled in another PR.

@gabro

This comment has been minimized.

Copy link
Member

commented Aug 13, 2019

@tdudzik so to summarize:

  • the lack of createdFiles will cause any empty open file to go through the package creation process? If so, I don't mind
  • good catch on the package.scala file, I agree it can be addressed in a separate PR

:shipit:

@tdudzik

This comment has been minimized.

Copy link
Author

commented Aug 13, 2019

  • the lack of createdFiles will cause any empty open file to go through the package creation process? If so, I don't mind

Exactly.

@tgodzik

This comment has been minimized.

Copy link
Collaborator

commented Aug 13, 2019

@tdudzik so to summarize:

  • the lack of createdFiles will cause any empty open file to go through the package creation process? If so, I don't mind
  • good catch on the package.scala file, I agree it can be addressed in a separate PR

:shipit:

The createdFiles could cause some race conditions and a situation where we open a new file that is empty is pretty much only when it's created. Besides it's easy to ctrl+z it.

Copy link
Collaborator

left a comment

Looks good. I left some minor suggestions, feel free to ignore them if you disagree :)

// package.scala file needs to be handled separately
// as its package name is different than package name of normal scala file
if (path.isScala &&
path.toFile.length() == 0 &&

This comment has been minimized.

Copy link
@marek1840

marek1840 Aug 13, 2019

Collaborator

why not move it to path.isEmpty or path.isEmptyFile in extensions?

Also, we can extract a part of the condition and get a oneliner:
if(isEmptyScalaFile(path) && path.filename != "pacakge.scala")

This comment has been minimized.

Copy link
@tgodzik

tgodzik Aug 13, 2019

Collaborator

Might change this in a follow up, but merging now, since everything is green.

val relativeDirectory = sourceItem
.map(path.toRelative)
.flatMap(relativePath => Option(relativePath.toNIO.getParent))
.map(_.toString)

This comment has been minimized.

Copy link
@marek1840

marek1840 Aug 13, 2019

Collaborator

maybe
map.(_.toString.replace("/", "."))

then we don't need the next statement

This comment has been minimized.

Copy link
@tgodzik

tgodzik Aug 13, 2019

Collaborator

That didn't work on windows - changed.

@tgodzik tgodzik merged commit 98d4302 into scalameta:master Aug 13, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
scalameta.metals Build #20190813.13 succeeded
Details
@tdudzik tdudzik deleted the tdudzik:auto_add_package branch Aug 14, 2019
@tdudzik tdudzik restored the tdudzik:auto_add_package branch Aug 14, 2019
@tdudzik tdudzik deleted the tdudzik:auto_add_package branch Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.