You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/10/11 15:29:37 UTC
[GitHub] [superset] villebro commented on a change in pull request #17056: refactor: Replaces areObjectsEqual with isEqualWith and adds default customizers
villebro commented on a change in pull request #17056:
URL: https://github.com/apache/superset/pull/17056#discussion_r726215281
##########
File path: superset-frontend/src/utils/isEqualCustomizers.test.ts
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEqualWith } from 'lodash';
+import {
+ undefinedCustomizer,
+ ignorePropsCustomizer,
+ compoundCustomizer,
+} from './isEqualCustomizers';
+
+test('ignores undefined with truthty result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 1, b: 2 }, undefinedCustomizer),
+ ).toBe(true);
+});
+
+test('ignores undefined with falsy result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 2, b: 2 }, undefinedCustomizer),
+ ).toBe(false);
+});
+
+test('ignores props with truthty result', () => {
Review comment:
nit:
```suggestion
test('ignores props with truthy result', () => {
```
##########
File path: superset-frontend/src/utils/isEqualCustomizers.test.ts
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEqualWith } from 'lodash';
+import {
+ undefinedCustomizer,
+ ignorePropsCustomizer,
+ compoundCustomizer,
+} from './isEqualCustomizers';
+
+test('ignores undefined with truthty result', () => {
Review comment:
nit:
```suggestion
test('ignores undefined with truthy result', () => {
```
##########
File path: superset-frontend/src/utils/isEqualCustomizers.ts
##########
@@ -0,0 +1,43 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { IsEqualCustomizer } from 'lodash';
+
+const undefinedCustomizer: IsEqualCustomizer = (value: any, other: any) =>
+ value === undefined || other === undefined ? true : undefined;
+
+const ignorePropsCustomizer = (...props: string[]): IsEqualCustomizer => (
+ value: any,
+ other: any,
+ key: string,
+) => (key && props.includes(key) ? true : undefined);
+
+const compoundCustomizer = (...customizers: IsEqualCustomizer[]) => (
+ value: any,
+ other: any,
+ key: string,
+) =>
+ customizers
+ .map(customizer =>
+ customizer(value, other, key, undefined, undefined, undefined),
+ )
+ .reduce((previousValue, currentValue) =>
+ previousValue || currentValue ? true : undefined,
+ );
Review comment:
Very cool, this will be very useful! While we're at it, we may want to have a customizer that doesn't distinguish between `null` and `undefined`. But maybe we can add that down the road when needed.
##########
File path: superset-frontend/src/utils/isEqualCustomizers.test.ts
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEqualWith } from 'lodash';
+import {
+ undefinedCustomizer,
+ ignorePropsCustomizer,
+ compoundCustomizer,
+} from './isEqualCustomizers';
+
+test('ignores undefined with truthty result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 1, b: 2 }, undefinedCustomizer),
Review comment:
I don't think this is what the `undefinedCustomizer` should do - IMO this should be `false`. However, this should be `true`:
```javascript
isEqualWith({ a: 1, b: undefined }, { a: 1}, undefinedCustomizer) === true
```
##########
File path: superset-frontend/src/utils/isEqualCustomizers.test.ts
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEqualWith } from 'lodash';
+import {
+ undefinedCustomizer,
+ ignorePropsCustomizer,
+ compoundCustomizer,
+} from './isEqualCustomizers';
+
+test('ignores undefined with truthty result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 1, b: 2 }, undefinedCustomizer),
+ ).toBe(true);
+});
+
+test('ignores undefined with falsy result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 2, b: 2 }, undefinedCustomizer),
Review comment:
While this assertion is correct, this should probably also be updated to catch the difference in `a`
##########
File path: superset-frontend/src/utils/isEqualCustomizers.test.ts
##########
@@ -0,0 +1,66 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { isEqualWith } from 'lodash';
+import {
+ undefinedCustomizer,
+ ignorePropsCustomizer,
+ compoundCustomizer,
+} from './isEqualCustomizers';
+
+test('ignores undefined with truthty result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 1, b: 2 }, undefinedCustomizer),
+ ).toBe(true);
+});
+
+test('ignores undefined with falsy result', () => {
+ expect(
+ isEqualWith({ a: 1, b: undefined }, { a: 2, b: 2 }, undefinedCustomizer),
+ ).toBe(false);
+});
+
+test('ignores props with truthty result', () => {
+ const customizer = ignorePropsCustomizer('b', 'c');
+ expect(
+ isEqualWith({ a: 1, b: 1, c: 1 }, { a: 1, b: 2, c: 2 }, customizer),
+ ).toBe(true);
+});
+
+test('ignores props with falsy result', () => {
+ const customizer = ignorePropsCustomizer('b', 'c');
+ expect(
+ isEqualWith({ a: 1, b: 1, c: 1 }, { a: 2, b: 2, c: 2 }, customizer),
+ ).toBe(false);
+});
+
+test('ignores undefined and props with truthty result', () => {
Review comment:
nit:
```suggestion
test('ignores undefined and props with truthy result', () => {
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org