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

Features/#277 create missing env file #445

Merged
merged 17 commits into from
Jan 15, 2022

Conversation

dakochik
Copy link
Contributor

@dakochik dakochik commented Dec 2, 2021

#277 Quick fix for missing .yaml / .yml file was implemented.

@iromeo iromeo self-requested a review December 3, 2021 11:24
@iromeo iromeo assigned dakochik and unassigned iromeo Dec 3, 2021
src/main/resources/SnakemakeBundle.properties Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
val targetName = tokens.lastOrNull() ?: return
var currentFile = file.virtualFile.parent
try {
tokens.forEach {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is reasonable to expect, that in such complicated application, like IntelliJ platform the task of creating file by relative path + create intermediate dirs isn't a rare case and already solved via some API. The code below is bicycle invention in a rather complected way. E.g. java.io already have methods for creating files and folders. So here we could use smth like:

        val targetFile = file.virtualFile.parent.findFileByRelativePath(fileName)
        // if targetFile == null ....
        val targetFileFolder = targetFile.parent
        VfsUtil.createDirectoryIfMissing(targetFileFolder.path)
        LocalFileSystem.getInstance().createChildFile(this, targetFileFolder, targetFile.name)

or just:

        val targetFile = Paths.get(file.virtualFile.parent.path, fileName)
        Files.createDirectories(targetFile.parent)
        Files.createFile(targetFile)

@iromeo
Copy link
Contributor

iromeo commented Dec 3, 2021

Also it is good to make this action undoable, try to use UndoableAction interface.

@iromeo
Copy link
Contributor

iromeo commented Dec 3, 2021

Allows to create files without YAML / YML / yaml / yml suffix, e.g. envs/ddd/ddd/dddaaaa , i think that better to forbid this and show error

@iromeo
Copy link
Contributor

iromeo commented Dec 3, 2021

Error should like failed to created /ednvs/... file, not untiled10
image

@iromeo iromeo added this to the next release milestone Dec 6, 2021
Dmitry added 6 commits December 11, 2021 14:14
…script, snakefile, configfile, pepfile, pepschema.

Resolves: #277
…ts tests running (and we're getting "RuntimeException: ... file is invalid: temp:///src/foo.smk ..."). So now we're just checking that there are available quickfix.

Resolves: #277
Resolves: #277
@dakochik dakochik assigned iromeo and unassigned dakochik Dec 18, 2021
Copy link
Contributor

@iromeo iromeo left a comment

Choose a reason for hiding this comment

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

Let's disscuss some impl details + need to fix absolute paths issue. Optionally you could file a new issue for abs path support and do not include it into this PR.

} catch (e: SecurityException) {
SmkNotifier.notify(
title = SnakemakeBundle.message("notifier.msg.create.env.file.title"),
content = SnakemakeBundle.message("notifier.msg.create.env.file.invalid.file.exception", fileName),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is error message is correct here? In which cases Security Exception occurs? Maybe you could show an error like cannot create file and add e.message as error details. Also, it is reasonable to log the exception error and message to the log file. E.g. see PythonParser.LOGGER as an example.

@@ -134,19 +137,13 @@ INSP.NAME.functions.not.allowed.in.section=Function object cannot be used as a v
# SmkWildcardNotDefinedInspection
INSP.NAME.wildcard.not.defined.title=Undefined wildcard usage.
INSP.NAME.wildcard.not.defined.in.section=Wildcard ''{0}'' isn''t defined in ''{1}'' section.
INSP.NAME.wildcard.not.defined.in.overridden.rule=Wildcard ''{0}'' isn''t defined in any appropriate section of overridden rules
INSP.NAME.wildcard.not.defined=Wildcard ''{0}'' isn''t properly defined.
INSP.NAME.wildcard.not.defined.cannot.check=Cannot check whether wildcard ''{0}'' is defined or not.
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually sounds to technical and not clear. If source file changed, that you could just show that source file changed. Please try again. Otherwise not clear what are "parts" of the file? Subdirectories?

val directoryPath = Files.createDirectories(targetFilePath.parent)
val directoryVirtualFile = VfsUtil.findFile(directoryPath, true) ?: return@Runnable
LocalFileSystem.getInstance().createChildFile(this, directoryVirtualFile, targetFilePath.name)
VirtualFileManager.getInstance().asyncRefresh { }
Copy link
Contributor

Choose a reason for hiding this comment

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

What for do we need refresh here? And why async? Is it a workaround to resart inspections in the current file? According to source code com.intellij.openapi.vfs.impl.local.LocalFileSystemBase.createChildFile does some events publishing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, not sure that we need to interact with Virtual File System here. 'undo' uses just "java.io.File" standard Java SDK API. Here maybe is better to check existence/create file w/o using VFS api, refresing, etc.

startElement: PsiElement,
endElement: PsiElement
) {
val relativeDirectory =
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix doesn't work ok if absolute path is used, e.g.:

rule rule277_s:
    conda: "/ednvs/ddd/ddd/dddaaaa.yaml"
    script: "env/ddddd.py"

on runtime expects absolute path:

$ snakemake --snakefile rule277.smk --use-conda -c1
Building DAG of jobs...
WorkflowError:
Failed to open source file /ednvs/ddd/ddd/dddaaaa.yaml
FileNotFoundError: [Errno 2] No such file or directory: '/ednvs/ddd/ddd/dddaaaa.yaml'

but quick fixes tries to create a file in project working directory. I assume that it valid for all file references for conda, script, etc sections. E.g. path could be absolute or relative. Is better to check.

@iromeo iromeo assigned dakochik and unassigned iromeo Dec 18, 2021
@dakochik
Copy link
Contributor Author

Discussed problems were solved.

@dakochik dakochik assigned iromeo and unassigned dakochik Dec 24, 2021
@iromeo iromeo merged commit 428c589 into master Jan 15, 2022
@dakochik dakochik deleted the features/#277-create-missing-env-file branch January 15, 2022 15:52
@iromeo iromeo modified the milestones: next release, 2022.1.743 May 4, 2022
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.

2 participants