villebro commented on a change in pull request #14461:
URL: https://github.com/apache/superset/pull/14461#discussion_r627324736
##########
File path: superset-frontend/src/dashboard/actions/nativeFilters.ts
##########
@@ -74,6 +74,7 @@ export const setFilterConfiguration = (
filterConfig,
});
const { id, metadata } = getState().dashboardInfo;
+ const oldFilters = getState().nativeFilters?.filters;
Review comment:
Ok, now I understand. With the introduction of yet a new property, the
properties in `fillNativeFilters` could probably use some renaming, as there's
lots of different state going through that function. Currently it's
```ts
function fillNativeFilters(
data: FilterConfiguration,
cleanState: DataMaskStateWithId,
draft: DataMaskStateWithId,
filters?: Filters,
) {
...
}
```
I think this would be clearer if it were renamed something like this:
```ts
function fillNativeFilters(
filterConfig: FilterConfiguration,
mergedDataMask: DataMaskStateWithId,
draftDataMask: DataMaskStateWithId,
currentFilters?: Filters,
) {
...
}
```
My point being, it's becoming increasingly difficult to follow the logic
here as it's getting more complex. So trying to be as explicit as possible when
naming variables can go a long way in making the code more readable.
FYI this is not a critique to this or other related code in general, I just
want to raise the point that any effort put into code readability can go a long
way helping others contribute to this code once the dust settles.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
