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.

Sign up for GitHub

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

Jump to bottom

US 5 // Filter resources that have path #1308

Merged
merged 3 commits into from
Jul 5, 2023
Merged

US 5 // Filter resources that have path #1308

merged 3 commits into from
Jul 5, 2023

Conversation

Dinika
Copy link
Contributor

@Dinika Dinika commented Jul 4, 2023

Screenshot from 2023-07-04 14-24-42

How has this been tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added necessary unit and integration tests.
  • I have added screenshots (if applicable), in the comment section.

@Dinika Dinika force-pushed the us-6/filter-resources-that-contain-value branch from 38cd34a to 10ff91c Compare July 4, 2023 13:13
@Dinika Dinika force-pushed the us-5/filter-resources-that-have-path branch from e6c2a92 to 68f4be7 Compare July 4, 2023 13:14
@Dinika Dinika force-pushed the us-6/filter-resources-that-contain-value branch from 10ff91c to 09091f6 Compare July 4, 2023 14:19
@Dinika Dinika force-pushed the us-5/filter-resources-that-have-path branch from 68f4be7 to 9d82891 Compare July 4, 2023 14:20
@codecov-commenter
Copy link

codecov-commenter commented Jul 4, 2023 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (us-6/filter-resources-that-contain-value@e00d2a3). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 9d82891 differs from pull request most recent head da6b3d4. Consider uploading reports for the commit da6b3d4 to get more accurate results

@@                             Coverage Diff                             @@
##             us-6/filter-resources-that-contain-value    #1308   +/-   ##
===========================================================================
  Coverage                                            ?   49.46%           
===========================================================================
  Files                                               ?      194           
  Lines                                               ?     8374           
  Branches                                            ?     1934           
===========================================================================
  Hits                                                ?     4142           
  Misses                                              ?     4205           
  Partials                                            ?       27

@Dinika Dinika changed the title Us 5 // Filter resources that have path US 5 // Filter resources that have path Jul 4, 2023
bilalesi
bilalesi approved these changes Jul 5, 2023
View reviewed changes
src/subapps/dataExplorer/PredicateSelector.tsx Outdated
export const isPathMissing = (
resource: { [key: string]: any },
path: string
): boolean => {
if (path in resource) {
if (path in resource && path !== '') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there situation where the path is empty string ?
If so is it better to filter paths when you generate them (just a suggestion)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this has been removed in a later PR. The path can never be empty ('').

src/subapps/dataExplorer/PredicateSelector.tsx Outdated
Comment on lines 122 to 150
export const DOES_NOT_EXIST = 'Does not exist';
export const EXISTS = 'Exists';
export const CONTAINS = 'Contains';
export type PredicateFilterT = typeof EMPTY_VALUE | typeof CONTAINS | null;

export type PredicateFilterT =
| typeof DOES_NOT_EXIST
| typeof EXISTS
| typeof CONTAINS
| null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest use this to be more Typescripty
const PREDICATE_VALUES = [DEFAULT_OPTION, DOES_NOT_EXIST, EXISTS, CONTAINS] as const;
type TPredicateValues = typeof PREDICATE_VALUES;
and then
type TPredicateValue = TPredicateValues[0 | 1 | 2] or TPredicateValues[number]
and then select by index when you need it TPredicateValues[index]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bon idee! I'll do that in a new PR.

src/subapps/dataExplorer/DataExplorer.tsx
predicatePath: string | null;
predicateFilter: PredicateFilterT | null;
predicateValue: string | null;
predicateFilter: ((resource: Resource) => boolean) | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it make sense if the predicateFilter can be rename it to predicate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this too.

@Dinika Dinika force-pushed the us-6/filter-resources-that-contain-value branch from 09091f6 to e00d2a3 Compare July 5, 2023 07:47
@Dinika Dinika force-pushed the us-5/filter-resources-that-have-path branch from d3ca970 to da6b3d4 Compare July 5, 2023 07:50
@Dinika Dinika merged commit 2781640 into us-6/filter-resources-that-contain-value Jul 5, 2023
1 check failed
@Dinika Dinika deleted the us-5/filter-resources-that-have-path branch July 5, 2023 07:51
@Dinika Dinika mentioned this pull request Jul 5, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bilalesi bilalesi bilalesi approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Dinika @codecov-commenter @bilalesi