You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2021/02/10 01:13:37 UTC
[GitHub] [incubator-pinot] zhangloo333 opened a new pull request #6568: Fix eslint error components fist 18
zhangloo333 opened a new pull request #6568:
URL: https://github.com/apache/incubator-pinot/pull/6568
## Description
Fix all eslint reported errors and warnings under app/pods/components
Test
Pass: yarn test
1..376
tests 376
pass 374
skip 2
fail 0
Pass: E2E test
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] zhangloo333 commented on a change in pull request #6568: Fix eslint error components fist 18
Posted by GitBox <gi...@apache.org>.
zhangloo333 commented on a change in pull request #6568:
URL: https://github.com/apache/incubator-pinot/pull/6568#discussion_r574014889
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-custom-baseline/component.js
##########
@@ -26,10 +22,11 @@ export default Component.extend({
selectedBaselineType: 'Unit over x units',
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
baselineTypes: [UNIT_OVER_UNIT, MEAN_UNIT, MEDIAN_UNIT, MAX_UNIT, MIN_UNIT],
selectedTimeUnit: 'Week',
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
I have checked that nearly all components have objects and arrays assigned to the property in the ember class. Because we will refactor all ember classes to native classes after upgrading to ember octan, it is safe to assign objects to fields in the native class (see [here](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/avoid-leaking-state-in-ember-objects.md)). I make a trade-off for skipping the ember object check currently and refactor all after migrating to ember octane. it will reduce repetitive work. We can club them in a constant folder to reduce some duplicate snippets or make it a constant in the file. Let me know your opinions on the plan.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] jihaozh merged pull request #6568: [TE]: Fix all eslint reported errors and warnings under app/pods/components
Posted by GitBox <gi...@apache.org>.
jihaozh merged pull request #6568:
URL: https://github.com/apache/incubator-pinot/pull/6568
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] tejasajmera commented on a change in pull request #6568: Fix eslint error components fist 18
Posted by GitBox <gi...@apache.org>.
tejasajmera commented on a change in pull request #6568:
URL: https://github.com/apache/incubator-pinot/pull/6568#discussion_r574768345
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-custom-baseline/component.js
##########
@@ -26,10 +22,11 @@ export default Component.extend({
selectedBaselineType: 'Unit over x units',
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
baselineTypes: [UNIT_OVER_UNIT, MEAN_UNIT, MEDIAN_UNIT, MAX_UNIT, MIN_UNIT],
selectedTimeUnit: 'Week',
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
I am not sure we will be updating the native components any time soon. I believe we want to update them as we go considering that the codebase is pretty sizable. But yeah since it is okay for a class to be having this and since each component ends up creating its own instance, the state should not be leaked. We can keep it the way it is.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
[GitHub] [incubator-pinot] tejasajmera commented on a change in pull request #6568: Fix eslint error components fist 18
Posted by GitBox <gi...@apache.org>.
tejasajmera commented on a change in pull request #6568:
URL: https://github.com/apache/incubator-pinot/pull/6568#discussion_r573969478
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-custom-baseline/component.js
##########
@@ -26,10 +22,11 @@ export default Component.extend({
selectedBaselineType: 'Unit over x units',
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
baselineTypes: [UNIT_OVER_UNIT, MEAN_UNIT, MEDIAN_UNIT, MAX_UNIT, MIN_UNIT],
selectedTimeUnit: 'Week',
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
You can set a `TIME_UNITS` constant above the component declaration and reference it here so you don't have to bypass eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-custom-baseline/component.js
##########
@@ -26,10 +22,11 @@ export default Component.extend({
selectedBaselineType: 'Unit over x units',
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
You can set a `BASELINE_TYPES` constant above the component declaration and reference it here so you don't have to bypass eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-select-comparison-range/component.js
##########
@@ -40,58 +37,64 @@ export default Component.extend({
showBaselineModal: false,
showForecastTimeRanges: false,
customBaselineValue: 'wo1w',
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
This entire object can be defined outside the component as a const and referenced here so as to avoid bypassing eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-select-comparison-range/component.js
##########
@@ -40,58 +37,64 @@ export default Component.extend({
showBaselineModal: false,
showForecastTimeRanges: false,
customBaselineValue: 'wo1w',
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
rangeOptions: {
'Last hour': [makeTime().subtract(1, 'hours').startOf('hour'), makeTime().startOf('hours').add(1, 'hours')],
'Last 3 hours': [makeTime().subtract(3, 'hours').startOf('hour'), makeTime().startOf('hours').add(1, 'hours')],
'Last 6 hours': [makeTime().subtract(6, 'hours').startOf('hour'), makeTime().startOf('hours').add(1, 'hours')],
'Last 24 hours': [makeTime().subtract(24, 'hours').startOf('hour'), makeTime().startOf('hours').add(1, 'hours')]
},
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
compareModeOptions: [
Review comment:
same as above.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/range-pill-selectors/component.js
##########
@@ -50,46 +50,35 @@ export default Component.extend({
/**
* A set of arbitrary time ranges to help user with quick selection in date-time-picker
*/
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
predefinedRanges: {
Review comment:
You can define this object as a constant outside the component and then reference it here to avoid having to bypass eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/rootcause-chart-toolbar/component.js
##########
@@ -44,9 +37,9 @@ export default Component.extend({
* @type {String[]} - array of string values of options
*/
granularityOptions: Object.keys(granularityMapping),
-
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
Lets define these arrays outside the component as constants and reference them here to avoid having to bypass eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/self-serve-alert-details/component.js
##########
@@ -38,6 +39,7 @@ export default Component.extend({
}
},
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
Lets define the object outside the component as a const to avoid bypassing the eslint.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/filter-bar-input/component.js
##########
@@ -26,31 +26,26 @@ import { computed } from '@ember/object';
import Component from '@ember/component';
export default Component.extend({
-
/**
* @type Array
* Default value for selected values in the filter bar input
*/
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
Same as above.
##########
File path: thirdeye/thirdeye-frontend/app/pods/components/entity-filter/component.js
##########
@@ -110,42 +106,42 @@ export default Component.extend({
* Array containing the running list of all user-selected filters
* @type {Array}
*/
+ // eslint-disable-next-line ember/avoid-leaking-state-in-ember-objects
Review comment:
You can do this to avoid having to bypass eslint here-
```suggestion
import { A as EmberArray } from '@ember/array';
alertFilters: EmberArray()
```
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org