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

FIX: display-edit dev build errors #106

Merged
merged 9 commits into from Jun 15, 2020

Conversation

mdelez
Copy link
Contributor

@mdelez mdelez commented Jun 11, 2020

@mdelez mdelez added the bug Something isn't working label Jun 11, 2020
@mdelez mdelez self-assigned this Jun 11, 2020
<dsp-uri-value class="parent-value-component" #displayVal *ngSwitchCase="constants.UriValue" [mode]="mode" [displayValue]="$any(displayValue)"></dsp-uri-value>
<dsp-decimal-value class="parent-value-component" #displayVal *ngSwitchCase="constants.DecimalValue" [mode]="mode" [displayValue]="$any(displayValue)"></dsp-decimal-value>
<dsp-color-value class="parent-value-component" #displayVal *ngSwitchCase="constants.ColorValue" [mode]="mode" [displayValue]="$any(displayValue)"></dsp-color-value>
<dsp-interval-value class="parent-value-component" #displayVal *ngSwitchCase="constants.IntervalValue" [mode]="mode" [displayValue]="$any(displayVal)"></dsp-interval-value>
Copy link
Contributor

Choose a reason for hiding this comment

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

$any(displayVal) ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed in 2f6f721

Copy link
Contributor

Choose a reason for hiding this comment

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

#displayVal is actually a variable, this is why this compiled. But since it is wrapped with $any(), no compatibility check was performed.

https://angular.io/guide/template-syntax#template-reference-variables-var

Copy link
Contributor

Choose a reason for hiding this comment

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

I wondered why the unit tests passed and checked the test in display-edit.component.spec:

it('should choose the apt component for an interval value in the template', () => {

      testHostComponent.assignValue('http://0.0.0.0:3333/ontology/0001/anything/v2#hasInterval');
      testHostFixture.detectChanges();

      expect(testHostComponent.displayEditValueComponent.displayValueComponent instanceof TestIntervalValueComponent).toBe(true);
      expect(testHostComponent.displayEditValueComponent.displayValueComponent.displayValue).not.toBeUndefined();
      expect(testHostComponent.displayEditValueComponent.displayValueComponent.mode).toEqual('read');
    });

expect(testHostComponent.displayEditValueComponent.displayValueComponent.displayValue).not.toBeUndefined(); should actually check for an instance of a ReadIntervalValue. That would make sure that the template correctly passed the ReadIntervalValue to the ReadIntervalValueComponent. The same applies to all other value types.

Shall we make a DSP task for this or do it in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

in case of $any(displayVal) a reference to TestIntervalValueComponent was passed instead of the ReadIn tervalValue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wondered why the unit tests passed and checked the test in display-edit.component.spec:

it('should choose the apt component for an interval value in the template', () => {

      testHostComponent.assignValue('http://0.0.0.0:3333/ontology/0001/anything/v2#hasInterval');
      testHostFixture.detectChanges();

      expect(testHostComponent.displayEditValueComponent.displayValueComponent instanceof TestIntervalValueComponent).toBe(true);
      expect(testHostComponent.displayEditValueComponent.displayValueComponent.displayValue).not.toBeUndefined();
      expect(testHostComponent.displayEditValueComponent.displayValueComponent.mode).toEqual('read');
    });

expect(testHostComponent.displayEditValueComponent.displayValueComponent.displayValue).not.toBeUndefined(); should actually check for an instance of a ReadIntervalValue. That would make sure that the template correctly passed the ReadIntervalValue to the ReadIntervalValueComponent. The same applies to all other value types.

Shall we make a DSP task for this or do it in this PR?

Good find! Since not much has changed in this PR in the end, I'm okay making the changes in this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot 2020-06-12 at 09 25 19

The test now correctly fails when using the wrong variable in the template

@tobiasschweizer
Copy link
Contributor

I have problems tracking this branch. SOmehow it doesn't like "FIX/" (capitalized).

<dsp-time-value class="parent-value-component" #displayVal *ngSwitchCase="constants.TimeValue" [mode]="mode" [displayValue]=$any(displayValue)></dsp-time-value>
<dsp-geoname-value class="parent-value-component" #displayVal *ngSwitchCase="constants.GeonameValue" [mode]="mode" [displayValue]=$any(displayValue)></dsp-geoname-value>
<dsp-link-value class="parent-value-component" #displayVal *ngSwitchCase="constants.LinkValue" [mode]="mode" [displayValue]=$any(displayValue)
<!-- display value is cast as 'any' because the compiler cannot infer the value type of the child component -->
Copy link
Contributor

Choose a reason for hiding this comment

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

"because the compiler cannot infer the value type of the child component" -> because the compiler cannot infer the value type expected by the child component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's clearer. Fixed in 2a89cfb

@mdelez
Copy link
Contributor Author

mdelez commented Jun 12, 2020

I have problems tracking this branch. SOmehow it doesn't like "FIX/" (capitalized).

weird, the other fix branches were named the same way

@tobiasschweizer
Copy link
Contributor

I have problems tracking this branch. SOmehow it doesn't like "FIX/" (capitalized).

weird, the other fix branches were named the same way

somehow my git could not track them correctly

@mdelez
Copy link
Contributor Author

mdelez commented Jun 12, 2020

I have problems tracking this branch. SOmehow it doesn't like "FIX/" (capitalized).

weird, the other fix branches were named the same way

somehow my git could not track them correctly

I'll switch to lowercase from now on :)

Copy link
Contributor

@tobiasschweizer tobiasschweizer left a comment

Choose a reason for hiding this comment

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

Good to go! Thanks.

@mdelez mdelez merged commit c8ad445 into master Jun 15, 2020
@mdelez mdelez deleted the FIX/DSP-389-display-edit-build-errors branch June 15, 2020 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants