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 module prefix when create new Haskell module #70
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zjhmale Some comments about your code. If you have questions let me know.
@@ -50,6 +56,54 @@ class CreateHaskellFileAction extends CreateFileFromTemplateAction(CreateHaskell | |||
}) | |||
} | |||
|
|||
|
|||
override def createFileFromTemplate(originName: String, template: FileTemplate, originDir: PsiDirectory): PsiFile = { | |||
val pathItems = HaskellFileUtil.getPathFromSourceRoot(originDir.getProject, originDir.getVirtualFile).orNull |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not use orNull
but keep it an option.
* Returns an array of directory names as the relative path to `file` from the source root. | ||
* For example, given file "project/src/foo/bar/baz.hs" the result would be `{"foo", "bar"}`. | ||
*/ | ||
@Nullable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove @Nullable
because that is Java. Scala has Option.
} | ||
} | ||
val result = for { | ||
project <- Option(project) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
project can not be null. Take a look to the code which retrieves it.
rootPath <- Option(root.getCanonicalPath) | ||
} yield loop(Some(file), rootPath) | ||
}.headOption | ||
result.flatten.flatten.flatten |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Three times flatten is a code smell. You can use flatMap
to prevent you have to flatten on the end.
val result = for { | ||
project <- Option(project) | ||
file <- Option(file) | ||
} yield ProjectRootManager.getInstance(project).getContentSourceRoots.view.map { root => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getContentSourceRoots can not be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, dir.getProject
and getContentSourceRoots
are all annotated with NotNull
.
if (defaultTemplateProperty != null) { | ||
PropertiesComponent.getInstance(project).setValue(defaultTemplateProperty, template.getName) | ||
} | ||
return psiFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use return
|
||
val psiFile = element.getContainingFile | ||
|
||
val virtualFile = psiFile.getVirtualFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Option wrapper.
val virtualFile = psiFile.getVirtualFile | ||
if (virtualFile != null) { | ||
FileEditorManager.getInstance(project).openFile(virtualFile, true) | ||
val defaultTemplateProperty = getDefaultTemplateProperty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Option wrapper
/** | ||
* Returns true if any directory name starts with a lower case letter. | ||
*/ | ||
def invalidPathItems(pathItems: List[String]): Boolean = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be private
} | ||
} catch { | ||
case e: ParseException => Messages.showErrorDialog(project, "Error parsing Velocity template: " + e.getMessage, "Create File from Template"); | ||
case e: Exception => throw e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be removed
return psiFile | ||
} | ||
} catch { | ||
case e: ParseException => Messages.showErrorDialog(project, "Error parsing Velocity template: " + e.getMessage, "Create File from Template"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also give template name.
I noticed that IntelliJ creates default template but that template is just Java code. Can we define a default Haskell template?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
O, wait there is already one created by me but in Settings/Editor/File and CodeTemplates
IntelliJ shows something else...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template is in src/main/resources/fileTemplates/internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there is a template named Haskell Module
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But why is the one in Settings/CodeTemplates not the same as the that is used when creating new file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rikvdkleij this should be fixed by #75.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool you investigated problem and fixed it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rikvdkleij You're welcome 😄
Cool, I'll fix these ASAP.
|
Mentioned at #69.
Implemented