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

Parse regular expression filter before storing to provenance graph #301

Merged
merged 4 commits into from
Feb 12, 2020

Conversation

oltionchampari
Copy link
Contributor

@oltionchampari oltionchampari commented Feb 5, 2020

Closes #306

Summary

  • The cause of the issue was applying JSON.stringify() to a RegExp object returns always {}
JSON.stringify({regexp: /^B/gm});
// result: "{\"regexp\":{}}"

which resulted in the provenance graph not properly storing the filter value.

  • Added a between step that:
    • transforms the regex filter to a string before storing it to the provenance graph
    • transforms it back to a regular expression when the replay happens.

 object to be propperly stored by provenance graph

Caleydo/tdp_bi_bioinfodb#1054
@oltionchampari oltionchampari added type: bug Something isn't working release: patch PR merge results in a new patch version labels Feb 5, 2020
@oltionchampari oltionchampari requested a review from thinkh February 5, 2020 12:17
@thinkh
Copy link
Member

thinkh commented Feb 5, 2020

@oltionchampari I improved some function and variable names and added more comments.

In my tests it works when I applying a regexp filter in the string column, but it does not work when I add another filter in the categorical column.

ordino-regexp-filter

cmds.ts:390 Uncaught (in promise) TypeError: Cannot read property 'isRegExp' of null
    at restoreRegExp (cmds.ts:390)
    at ActionNode.<anonymous> (cmds.ts:201)
    at Generator.next (<anonymous>)
    at fulfilled (tslib.es6.js:62)

Please fix this behavior and test it more deeply in combination with other filters and jumping back and forth in the provenance graph.

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

See previous comment.

@oltionchampari
Copy link
Contributor Author

See previous comment.

Thanks for the feedback and the further optimizations you made!

The problem above was that i wasn't checking for null values.
After extensive testing everything seems to be working as expected.

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

@oltionchampari I still get an error when I play around with different filters.

ordino-regexp-filter

Please test the following provenance graph:

Temporary Session 266.json.txt

@oltionchampari
Copy link
Contributor Author

@oltionchampari I still get an error when I play around with different filters.

ordino-regexp-filter

Please test the following provenance graph:

Temporary Session 266.json.txt

@thinkh Cant reproduce it. Maybe you don't have the latest branch .

test

@thinkh
Copy link
Member

thinkh commented Feb 12, 2020

Yes, indeed it works!

Copy link
Member

@thinkh thinkh left a comment

Choose a reason for hiding this comment

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

Thanks! Works and looks good.

@thinkh
Copy link
Member

thinkh commented Feb 12, 2020

@oltionchampari Could please try one more time, if the provenance graph with a value: {} in the action will not break anymore.

Because stringify is missleading as it does not retrun a string, but an object.
@oltionchampari
Copy link
Contributor Author

@oltionchampari Could please try one more time, if the provenance graph with a value: {} in the action will not break anymore.

The current solution also works for filter value: {}. The filter value: {} is set and the rest of the prov graph steps continue running.

@thinkh thinkh merged commit 33fcd4d into develop Feb 12, 2020
@thinkh thinkh deleted the ochampari/1054-regular_expression-filter-bugfix branch February 12, 2020 15:41
@dvvanessastoiber dvvanessastoiber mentioned this pull request Mar 18, 2020
29 tasks
@ghost ghost mentioned this pull request Mar 19, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: patch PR merge results in a new patch version type: bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants