Exclude #61
Exclude #61
Conversation
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR removes the Regex filter mode and simplifies filtering to use Include/Exclude modes with AdvancedFilter instead of BasicFilter. The changes migrate existing regex settings to the Include mode and update filtering logic to use the "Contains" operator.
Key changes:
- Replaced BasicFilter with AdvancedFilter for more sophisticated filtering capabilities
- Removed Regex filter mode and related multi-selection functionality
- Added migration logic to handle existing regex settings
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/visual.ts | Refactored filtering logic to use AdvancedFilter with Contains/DoesNotContain operators, removed regex search implementation and multi-selection handling |
| src/settings.ts | Removed regex filter mode option and multi-selection settings group, added migration method for existing settings |
| src/filterMode.ts | Commented out Regex mode from FilterModeOptions array |
| package.json | Updated dependency versions for various packages |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/visual.ts
Outdated
|
|
||
|
|
||
| this.migrateRegexSetting(); | ||
| this.formattingSettings.migrateRegexSetting() |
There was a problem hiding this comment.
Line 196 is missing a semicolon at the end of the statement, which is inconsistent with the code style used throughout the file.
| this.formattingSettings.migrateRegexSetting() | |
| this.formattingSettings.migrateRegexSetting(); |
src/visual.ts
Outdated
| if ("conditions" in advancedFilters[0]) { | ||
| searchText = advancedFilters[0].conditions[0].value.toString(); | ||
| } else if ("values" in advancedFilters[0]) { |
There was a problem hiding this comment.
This code assumes advancedFilters has at least one element and that conditions/values arrays are non-empty without any validation. This will throw a runtime error if advancedFilters is an empty array or if conditions[0]/values[0] don't exist. Add array length checks before accessing indices.
| if ("conditions" in advancedFilters[0]) { | |
| searchText = advancedFilters[0].conditions[0].value.toString(); | |
| } else if ("values" in advancedFilters[0]) { | |
| if ("conditions" in advancedFilters[0] && Array.isArray(advancedFilters[0].conditions) && advancedFilters[0].conditions.length > 0 && advancedFilters[0].conditions[0].value !== undefined) { | |
| searchText = advancedFilters[0].conditions[0].value.toString(); | |
| } else if ("values" in advancedFilters[0] && Array.isArray(advancedFilters[0].values) && advancedFilters[0].values.length > 0 && advancedFilters[0].values[0] !== undefined) { |
| const isBlank = ((text || "") + "").match(/^\s*$/); | ||
| if (!this.column) { | ||
| return; | ||
| } | ||
| this.searchBox.property("value", text); | ||
| if (isBlank) { |
There was a problem hiding this comment.
The variable 'isBlank' is defined but never used after lines 287-289 are reached. The early returns make this check redundant. Consider removing the variable declaration and simplifying the logic.
| const isBlank = ((text || "") + "").match(/^\s*$/); | |
| if (!this.column) { | |
| return; | |
| } | |
| this.searchBox.property("value", text); | |
| if (isBlank) { | |
| if (!this.column) { | |
| return; | |
| } | |
| if (((text || "") + "").match(/^\s*$/)) { |
There was a problem hiding this comment.
this looks cleaner
src/visual.ts
Outdated
| let filter: AdvancedFilter | BasicFilter | null = null; | ||
| let action: FilterAction = FilterAction.merge; | ||
|
|
||
| matches = [text] |
There was a problem hiding this comment.
[nitpick] The 'matches' variable is declared on line 298 but only assigned once on line 302, and is immediately used inline on line 303. Consider removing the variable declaration and directly using 'text' in the AdvancedFilter constructor for clarity.
| private regexSearch(text: string): string[] { | ||
| const regex = new RegExp(text); | ||
| const values: string[] = this.dataView?.categorical?.categories?.[0]?.values.map(x => x.toString()) || []; | ||
| const values: string[] = Array.from(new Set(this.dataView?.categorical?.categories?.[0]?.values.map(x => x.toString()) || [])); | ||
| const filteredValues = values.filter(value => regex.test(value)); | ||
| return filteredValues | ||
| } |
There was a problem hiding this comment.
The 'regexSearch' method is no longer called anywhere in the code after removing regex filter mode support. This dead code should be removed to improve maintainability.
There was a problem hiding this comment.
We will need it in the future
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
| @@ -1,3 +1,7 @@ | |||
| ## 2.4.3.0 | |||
There was a problem hiding this comment.
align the version with other configuration
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
Signed-off-by: REDMOND\v-bdjalalov <[email protected]>
2 participants
No description provided.