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/#360, #433 PEPs configuration support #424

Merged
merged 31 commits into from
Oct 31, 2021

Conversation

KrGolovin
Copy link
Contributor

@KrGolovin KrGolovin commented Aug 16, 2021

See issues: #433, #360

@iromeo iromeo assigned KrGolovin and unassigned iromeo Aug 16, 2021
Kirill Golovin added 9 commits August 17, 2021 13:48
# Conflicts:
#	src/main/kotlin/com/jetbrains/snakecharm/inspections/SmkIgnorePyInspectionExtension.kt
#	src/main/kotlin/com/jetbrains/snakecharm/lang/SnakemakeNames.kt
#	src/main/kotlin/com/jetbrains/snakecharm/lang/parser/SmkTokenTypes.kt
#	src/main/kotlin/com/jetbrains/snakecharm/lang/parser/SnakemakeLexer.kt
#	src/main/kotlin/com/jetbrains/snakecharm/lang/psi/SmkFile.kt
#	src/main/kotlin/com/jetbrains/snakecharm/lang/psi/impl/SmkWorkflowArgsSectionImpl.kt
Also fixed completion and resolve to pep
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.

Completion list missing some variants (nested yaml items, e.g. 'key1') and allows non-identifiers (e.g. key3:key31) that leads to syntax errors in Snake file

Pipeline:

# pep1/config_360.yaml
pep_version: "2.0.0"
sample_table: ../config/sample_table.tsv
donor_table: ../config/donor_table.tsv

key1:
  key11: 
    "val11"
key2:
  - key21: "val21"
key3:key31: "val31"
key4.key41: "val41"
key5.key51:
  key52: "val52"
sample_modifiers:
  derive:
    attributes: [file_path]
    sources:
      source1: /data/lab/project/{organism}_{time}h.fastq
      source2: /path/from/collaborator/weirdNamingScheme_{external_id}.fastq
      
'not_ident: $$ ifier':
  sss: "ddd"
      
# ------ Snakefile --------
pepfile: "pep1/config_360.yaml"

print("--- All keys: ----")
print(pep.config)
print("--- key1: ----")
print(pep.config.key1)
print(pep.config.key1.key11)
print(pep.config['key1'])
print(pep.config['key1'].key11)
print(pep.config.key1['key11'])
print("--- key2: ----")
print(pep.config.key2)
print(pep.config.key2[0]['key21'])
print("--- key3: ----")
print(pep.config['key3:key31'])
# print(pep.config['key3']) # <- KeyError in line .. 'key3'
# print(pep.config.key3) # <- AttributeError in line .. 'key3'
print("--- key4: ----")
print(pep.config['key4.key41'])
# print(pep.config.key4) # <- AttributeError in line .. 'key4'
# print(pep.config.key4.key41) # <- AttributeError in line .. 'key4'
print("--- key5: ----")
print(pep.config['key5.key51'])
print(pep.config['key5.key51'].key52)
print("--- sample_modifiers: ----")
print(pep.config.sample_modifiers.derive)
print(pep.config['not_ident: $$ ifier'])

rule all:

Output:

$ snakemake --snakefile rule_360/rule_360.smk -c1 all  -n -q
--- All keys: ----
PathExAttMap
pep_version: 2.0.0
sample_table: /Users/romeo/work/snakecharm/snakemaek_examples/untitled10/pep1/../config/sample_table.tsv
donor_table: ../config/donor_table.tsv
key1:
  key11: val11
key2: 
 - {'key21': 'val21'}
key3:key31: val31
key4.key41: val41
key5.key51:
  key52: val52
--- key1: ----
PathExAttMap
key11: val11
val11
PathExAttMap
key11: val11
val11
val11
<...>
--- key2: ----
[{'key21': 'val21'}]
val21
--- key3: ----
val31
--- key4: ----
val41
--- key5: ----
PathExAttMap
key52: val52
val52
--- sample_modifiers: ----
PathExAttMap
attributes: 
 - file_path
sources:
  source1: /data/lab/project/{organism}_{time}h.fastq
  source2: /path/from/collaborator/weirdNamingScheme_{external_id}.fastq
  
PathExAttMap
sss: ddd

return completionList
}

private fun getYamlFile(smkFile: SmkFile): PsiFile? {
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 assume that getYamlFile(..) should return not any PsiFile? (e.g. PyFile, SmkFile, etc) but only YAMLFile or null.

So the signature of this method could be improoved.

val ymlFile = yamlFile as YAMLFile
val completionList = mutableListOf<PsiElement>()
ymlFile.documents.forEach { document ->
document.topLevelValue?.children?.forEach { child ->
Copy link
Contributor

Choose a reason for hiding this comment

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

We've already discussed several times that low-level PSI API access is not recommended and better to use high level API instead. Especially when you deal with some 3-rd party PSI elements where parser impl could be changed at any time and such low-level code will stop working. Please find appropriate YAML PSI api and use it instead of low level API.

}

private fun getYamlFile(smkFile: SmkFile): PsiFile {
val psiElement = smkFile.findPepfile()!!.getSectionKeywordNode() as LeafPsiElement
Copy link
Contributor

Choose a reason for hiding this comment

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

The whole method 1) is using low level PSI access and parser impl details 2) duplicates some code from SmkPepfileReference reference 3) doesn't work properly if reference uses single quoting

Re-implement this using high-level PSI access (e.g. without LeafPsiElement, nextSibling, lastChild, text methods). Use the fact, that if everyting is ok, you already have SmkPepfileReference reference here and it is know how to resolve it's target, no need to copy resolve implementation here. Before commiting this change please show me new impl in Slack.

// like pep.config.foo, but not pep.config.foo.boo
child.firstChild to
child.firstChild.text.substringBefore('.')
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not all items are valid for completion list, e.g. key1:key2 is invalid option, because pep.config.key1:key2 is a syntax error. Make code smarter and add to completion list only valid identifiers

@iromeo iromeo removed their assignment Aug 20, 2021
@iromeo iromeo assigned iromeo and unassigned KrGolovin Oct 31, 2021
@iromeo
Copy link
Contributor

iromeo commented Oct 31, 2021

  1. The cache is organized as SmkPepConfigCollector object with private static field cache. This is memory leak & not threads safe. Should be changed.

  2. pepfile, pepschema isn't in keyword comletion

  3. seems reference completion after pepfile: "<>" is very slow.

@iromeo iromeo changed the title Features/#360 PEPs configuration support Features/#360, #433 PEPs configuration support Oct 31, 2021
@iromeo
Copy link
Contributor

iromeo commented Oct 31, 2021

#360 is done, merged into master. #418 code was disabled in 9743246 commit

@iromeo iromeo merged commit 839a875 into master Oct 31, 2021
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