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

some rules report false positives #379

Closed
rolandjitsu opened this issue Jul 17, 2017 · 17 comments · Fixed by #381
Closed

some rules report false positives #379

rolandjitsu opened this issue Jul 17, 2017 · 17 comments · Fixed by #381
Assignees
Labels

Comments

@rolandjitsu
Copy link

I just discovered some weird cases where angular-whitespace complains about:

  • <div fxLayout="column"> (the l - as in the letter L - must be the confusing part)
  • <md-icon *ngIf="isOnline | async; else offline">cloud_off</md-icon>
  • <md-toolbar color="primary"> - might be the same case as the first one
  • <rj-image-filters (queryChange)="updateFilters($event)"></rj-image-filters> - could be the same case with the letter L
  • <!-- Setttings --> - might be the ! sign there, nonetheless, comments should be ignored
  • <md-select [(ngModel)]="timeRangeKey"> - could be the L again
  • *ngIf="query.isUpdating | async as isUpdating" - there's obviously space there, so not sure what the problem is

And banana-in-a-box:

  • <a md-list-item (click)="navigate(['/resources'])"> - confuses what's inside the quotes I guess

Finally no-access-missing-member :

  • complains about a prop not existing on the class, but it's actually a local var
<rj-paginated-resources *ngIf="query"
							#pagination="rjPagination"
							[type]="type"
							[query]="query | async">
	<div fxLayout="row"
		 fxLayoutWrap
		 class="images"
		 padding>
		<ng-container *ngFor="let image of pagination.items | async; trackBy: trackByImage">
			<div class="image">
				<rj-image-card [image]="image"></rj-image-card>
			</div>
		</ng-container>
	</div>
</rj-paginated-resources>
@wKoza
Copy link
Collaborator

wKoza commented Jul 18, 2017

Thank you for your issue report. For the last point, there are many issues in the tracker related to template references. For the others, we will investigate.

@wKoza
Copy link
Collaborator

wKoza commented Jul 18, 2017

Can you give me an example of log for this item : <div fxLayout="column"> ?

Concerning the check-pipe, can you remove it form your configuration for this moment because there is an issue opened (issue #365 ).

@rolandjitsu
Copy link
Author

Definitely:

/users/rolandjitsu/projects/test/src/app/test.component.html
ERROR: 4:4    angular-whitespace        The pipe operator should be surrounded by one space on each side, i.e. " | ".

@wKoza
Copy link
Collaborator

wKoza commented Jul 18, 2017

And if you remove the check-pipe in your lint, what's happening ?

@rolandjitsu
Copy link
Author

If I remove that, I get no errors 😄

@wKoza
Copy link
Collaborator

wKoza commented Jul 19, 2017

Ok nice. You can monitor this issue for resolution. Otherwise, there is indeed a problem with the rule "banana-in-a-box". I will take a look at this weekend.

@rafaelss95
Copy link
Collaborator

rafaelss95 commented Aug 3, 2017

Maybe it worth to create a new issue for this, but since the title is a bit generic, let's report it here:

Having the following:

@Component({
  ...
  template: `
    <div *ngIf="authService.isLoggedIn()">
     ...
    </div>
  `
})
export class AppComponent {

  constructor(private authService: AuthService) { }

}

Results in:

You can bind only to public class members. "authService" is not a public class member.

@mgechev
Copy link
Owner

mgechev commented Aug 3, 2017

That's the expected behavior from the rule template-use-public.

@mgechev
Copy link
Owner

mgechev commented Aug 8, 2017

Lets reopen the issue since there are problems in other rules (angular-whitespace has some flaws).

@mgechev mgechev reopened this Aug 8, 2017
@mgechev
Copy link
Owner

mgechev commented Aug 12, 2017

The problem in all rules which involve EmbeddedTemplate is that the syntax is being desugared from:

<li *ngFor="let foo of bar"></li>

to:

<li ngFor let-foo [ngForOf]="bar"></li>

which breaks our mapping. Here's the solution by the language service. It's not straightforward. Most likely I'll reuse it, however, we risk to have frequent breaking changes given the non-stable APIs being used there.

@wKoza
Copy link
Collaborator

wKoza commented Aug 12, 2017

mmhm, i hope this will be okay ...

@mgechev
Copy link
Owner

mgechev commented Aug 12, 2017

I'm afraid we'll need to keep this. It can be implemented with a strategy, doesn't have to be in a long function but the logic will remain.

@wKoza
Copy link
Collaborator

wKoza commented Aug 28, 2017

Hi @mgechev, have you progress in this issue ?

@mgechev
Copy link
Owner

mgechev commented Aug 28, 2017

Haven't done anything here yet. The way to go is custom expression parser for *-prefixed attributes.

@wKoza
Copy link
Collaborator

wKoza commented Aug 29, 2017

ok, I'll look at this If I have time.

@rolandjitsu
Copy link
Author

I'm still seeing some problems with the check-pipe rule for:

<p *ngIf="(items | async)?.length as length" class="mat-caption">
<span *ngIf="product.propagationTolerance" class="label">&plusmn; {{ product.propagationTolerance | number }}</span>

For the latter, removing &plusmn; seems to fix it.

@mgechev
Copy link
Owner

mgechev commented Sep 12, 2017

@rolandjitsu thanks for pointing this out! I'll move it to a separate issue. Hopefully we'll be able to soon clean the glitches and open a PR to Angular CLI with the new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants