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

Adding ability to specify a filter with REMOVE #124

Merged
merged 1 commit into from
Mar 12, 2020
Merged

Conversation

vsoch
Copy link
Member

@vsoch vsoch commented Mar 12, 2020

Currently, REMOVE can handle a func:, however since we are returning a boolean, we can easily also support the already existing filters that return a boolean (contains, equals, etc.) This
change will allow for those filters. This will address #123, pinging @wetzelj

Signed-off-by: vsoch vsochat@stanford.edu

Questions that require more discussion or to be addressed in future development:

Copy link
Contributor

@wetzelj wetzelj left a comment

Choose a reason for hiding this comment

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

I'm having a hard time figuring out the purpose of the missing, present, and empty expanders in this situation - and what the expected results should be. In my testing REMOVE ALL empty removed all tags. I was left with a header that contained only those items that I added back in after performing the REMOVE ALL empty.

My expectation would be that REMOVE ALL empty would remove tags that contained empty values. I don't quite understand how present or missing would apply to values.

@vsoch
Copy link
Member Author

vsoch commented Mar 12, 2020

Yeah I was just thinking about this too - I was grabbing the filters from the list already derived, but I don't think those particular filters make sense. I'm going to remove them.

@vsoch vsoch closed this Mar 12, 2020
@vsoch vsoch reopened this Mar 12, 2020
@vsoch
Copy link
Member Author

vsoch commented Mar 12, 2020

oops didn't mean to close it.

Currently, REMOVE <field> can handle a func:, however since we are
returning a boolean, we can easily also support the already existing
filters that return a boolean (contains, equals, etc.) This
change will allow for those filters.

Signed-off-by: vsoch <vsochat@stanford.edu>
Copy link
Contributor

@wetzelj wetzelj left a comment

Choose a reason for hiding this comment

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

Looks good!

@vsoch vsoch merged commit 91f06a1 into master Mar 12, 2020
@vsoch
Copy link
Member Author

vsoch commented Mar 12, 2020

@vsoch
Copy link
Member Author

vsoch commented Mar 12, 2020

@wetzelj let me think about your comment:

Regarding the plan forward... #118 is really one of the higher priority issues from our standpoint, but if you think that #119 should be done first, that's okay too.

In that adding private tags will mean parsing over the tag objects (which reveal data elements) I'm actually thinking that these two separate issues need to be addressed at the same time - we will be able to easily loop over private tags if the get/set is available. I can't promise starting on this today, but soon!

@wetzelj
Copy link
Contributor

wetzelj commented Mar 12, 2020

No problem! I appreciate all of your work on this!

@vsoch vsoch deleted the add/remove-filters branch March 12, 2020 20:05
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.

None yet

2 participants