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

Fix alignments read filter jexl syntax #1860

Merged
merged 3 commits into from
Mar 31, 2021
Merged

Fix alignments read filter jexl syntax #1860

merged 3 commits into from
Mar 31, 2021

Conversation

cmdcolin
Copy link
Collaborator

This fixes the filters on alignments track e.g. read name, which currently had jexl syntax errors which are displayed in 1.1.0 if filters attempt to be used

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Mar 31, 2021
@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Mar 31, 2021
@codecov
Copy link

codecov bot commented Mar 31, 2021

Codecov Report

Merging #1860 (9c70dc6) into main (1ed6ae8) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1860      +/-   ##
==========================================
- Coverage   59.18%   59.17%   -0.01%     
==========================================
  Files         460      460              
  Lines       21402    21402              
  Branches     5024     5024              
==========================================
- Hits        12667    12665       -2     
- Misses       8428     8430       +2     
  Partials      307      307              
Impacted Files Coverage Δ
...lugins/alignments/src/LinearPileupDisplay/model.ts 69.02% <ø> (ø)
...ments/src/LinearSNPCoverageDisplay/models/model.ts 69.23% <ø> (ø)
...inearGenomeView/components/RefNameAutocomplete.tsx 89.36% <0.00%> (-4.26%) ⬇️
packages/core/util/index.ts 80.99% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed6ae8...9c70dc6. Read the comment docs.

@rbuels
Copy link
Contributor

rbuels commented Mar 31, 2021

Is there anything we can do in the code to make these jexl syntax errors get detected sooner? cc @peterkxie

Could maybe make a jexl() util function that just returns the passed string, but also syntax checks it if inDevelopment?

@cmdcolin
Copy link
Collaborator Author

cmdcolin commented Mar 31, 2021

probably just need more unit/integration testing, not sure there is a nice way to "automatically detect a syntax error" here

@peterkxie
Copy link
Contributor

I think more unit/integration testing is the best solution, with us changing jexl from functions -> transforms -> functions that is probably the source of most syntax errors. After catching the initial batch it should be easier now that we are sticking with functions

@rbuels rbuels merged commit 322c211 into main Mar 31, 2021
@rbuels rbuels deleted the fix_filter_jexl branch March 31, 2021 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants