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.

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

Falsy truthy and fix doov function #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tretail
Copy link

@tretail tretail commented Dec 17, 2019

Porter un point d'attention particulier sur les modifs dans doov.ts ; on est bien d'accord que les BooleanFunction retournent toutes un boolean ?

@codecov-io
Copy link

codecov-io commented Dec 17, 2019

Codecov Report

Merging #37 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage   99.84%   99.84%   +<.01%     
==========================================
  Files          47       47              
  Lines        1277     1285       +8     
  Branches      166      166              
==========================================
+ Hits         1275     1283       +8     
  Partials        2        2
Impacted Files Coverage Δ
src/dsl/lang/BooleanFunction.ts 100% <100%> (ø) ⬆️
src/dsl/lang/DefaultOperators.ts 100% <100%> (ø) ⬆️
src/doov.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34f8a82...15767df. Read the comment docs.

src/doov.ts Outdated
@@ -167,7 +167,8 @@ export function sum(...values: NumberFunction[]): NumberFunction {
return new NumberFunction(new NaryMetadata(values.map(value => value.metadata), SUM), (obj, ctx) => {
return values.reduce((previous, value) => {
const v = value.get(obj, ctx);
return v != null ? previous + v : previous;
// @ts-ignore : Here we are sure that v is neither null or undefined since Boolean(v) would return false. Furthermore, previous + 0 <=> previous.
Copy link
Member

Choose a reason for hiding this comment

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

Replace that with not null && not undefined ? previous + v : previous

src/doov.ts Outdated
@@ -158,7 +158,7 @@ export function count(...values: BooleanFunction[]): NumberFunction {
return new NumberFunction(new NaryMetadata(values.map(value => value.metadata), COUNT), (obj, ctx) => {
return values.filter(value => {
const v = value.get(obj, ctx);
return v != null ? v : false;
return Boolean(v) || false;
Copy link
Member

Choose a reason for hiding this comment

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

Remove || false

src/doov.ts Outdated
@@ -149,7 +149,7 @@ export function matchNone(...values: BooleanFunction[]): BooleanFunction {
return new BooleanFunction(new NaryMetadata(values.map(value => value.metadata), NONE_MATCH), (obj, ctx) => {
return values.every(value => {
const v = value.get(obj, ctx);
return v != null ? !v : false;
return !v || false;
Copy link
Member

Choose a reason for hiding this comment

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

!Boolean(v)

public isTruthy(): BooleanFunction {
return new BooleanFunction(
new UnaryMetadata(this.metadata, IS_TRUTHY),
condition(this, false, (_: boolean) => true, false)
Copy link
Member

Choose a reason for hiding this comment

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

(expr: boolean) => Boolean(expr), false)

public isFalsy(): BooleanFunction {
return new BooleanFunction(
new UnaryMetadata(this.metadata, IS_FALSY),
condition(this, false, (expr: boolean) => !Boolean(expr), true)
Copy link
Member

Choose a reason for hiding this comment

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

✔️

@tretail tretail force-pushed the falsy_truthy_and_fix_doov_function branch from 9678886 to 15767df Compare December 20, 2019 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants