Skip to content

Conversation

@asizon
Copy link
Member

@asizon asizon commented Jun 6, 2021

This PR adds new filter type options to PidTuning/Filters Configurator tab, to go with a small part of betaflight/betaflight#10727

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

It's ok to me.

Comment on lines +1397 to +1400
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
filterTypeValues.push("PT2");
filterTypeValues.push("PT3");
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if I like more this way or only with one push...

Suggested change
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
filterTypeValues.push("PT2");
filterTypeValues.push("PT3");
}
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
filterTypeValues.push("PT2", "PT3");
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting, but maybe i like the first way to best visual differentation betwen two filters type. Do you think that with a single push the performance will be better?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not too sure what I prefer, only was a comment 🙃

Copy link
Member

Choose a reason for hiding this comment

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

I prefer the former as it's easier to understand 🙃

@haslinghuis
Copy link
Member

haslinghuis commented Jun 9, 2021

I've to change PTx in 4 places. Can't this be done in one place?

@haslinghuis
Copy link
Member

We need one test and the third approval here to test the new process. Milestone and RN tag is already set.

@haslinghuis
Copy link
Member

haslinghuis commented Jun 10, 2021

@blckmn approver count passed? It's two at the moment 😕 The process should not run yet?

@blckmn
Copy link
Member

blckmn commented Jun 10, 2021

AUTOMERGE: (PASS)

  • Github identifies PR as mergeable -> PASS
  • PR is not in draft -> PASS
  • Assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • 'Don't merge' label NOT found -> PASS
  • At least one 'RN:' label found -> PASS
  • 'Tested' label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> PASS

@asizon
Copy link
Member Author

asizon commented Jun 10, 2021

@haslinghuis I dont understund what are you refering with two places.

@blckmn
Copy link
Member

blckmn commented Jun 10, 2021

@blckmn approver count passed? It's two at the moment 😕 The process should not run yet?

I am running it manually at the moment and doing testing :)

@blckmn
Copy link
Member

blckmn commented Jun 10, 2021

Found the problem with the reviewers. If you submit two reviews, it adds you twice. Now counting the unique users who have approved the PR :)

@blckmn blckmn self-requested a review June 10, 2021 07:31
@haslinghuis haslinghuis self-assigned this Jun 10, 2021
@blckmn blckmn merged commit 9b4a422 into betaflight:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants