Skip to content

Update TranslateComponent.ts #49

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

Closed
wants to merge 3 commits into from

Conversation

BorntraegerMarc
Copy link
Contributor

To fix (angular's default) tslint errors. These errors should definitely be fixed, because with tslint it is currently not possible to exclude folders when running ng lint. So these tslint errors cause our CI build to fail...

These were the errors:

ERROR: D:/projects/komed-health-web/node_modules/angular-translator/src/TranslateComponent.ts[10, 16]: Missing whitespace in interpolation; expecting {{ expr }}
ERROR: D:/projects/komed-health-web/node_modules/angular-translator/src/TranslateComponent.ts[9, 15]: The selector of the component "TranslateComponent" should be used as element (https://angular.io/st
yleguide#style-05-03)
ERROR: D:/projects/komed-health-web/node_modules/angular-translator/src/TranslateComponent.ts[11, 5]: Use the @Input property decorator instead of the inputs property (https://angular.io/styleguide#sty
le-05-12

To fix (angular's default) tslint errors
@BorntraegerMarc
Copy link
Contributor Author

@tflori could you please publish a new version, once this is resolved? :)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 596757b on BorntraegerMarc:master into 7ac673c on tflori:master.

@BorntraegerMarc
Copy link
Contributor Author

my suggestion is to change the failing tests / tslint config from travis. Because for most of the users the tslint will fail because they use the default one from angular...

@tflori
Copy link
Owner

tflori commented Jul 19, 2017

I'm fine with changing the tslint configuration. So if build works I can release a new version..

Sent from my OnePlus ONEPLUS A3003 using FastHub

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 2592d8e on BorntraegerMarc:master into 7ac673c on tflori:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f2273e1 on BorntraegerMarc:master into 7ac673c on tflori:master.

@BorntraegerMarc
Copy link
Contributor Author

@tflori One test still fails but I don't really know why. Could you help me out?

@tflori
Copy link
Owner

tflori commented Jul 20, 2017

I don't understand why you changed TranslateParams but not tslint.json...

Is it necessary to have the @input declaration in front of the setters now?

@tflori
Copy link
Owner

tflori commented Jul 20, 2017

I think the biggest problem will be the setting no-eval because without this the Translator.parse() method will throw an error. I think the problem is located in ng cli that does not exclude node_modules directory...

If you find a way without failing tests it would be great. I'm currently working on other projects and can't really help on this minor issue (from my point of view).

@tflori
Copy link
Owner

tflori commented Jul 20, 2017

Is the only problem the selector of translate? Try moving the Input specification to @component specification again. Maybe this helps.

I'm still wondering if ng lint checks the code of angular-translator?

Sent from my OnePlus ONEPLUS A3003 using FastHub

@BorntraegerMarc
Copy link
Contributor Author

Thanks for the feedback. Changing the tslint.json wouldn't help because projects using the translator would have their own tslint.json and would override it. This is my source for tslint exclude not working: palantir/tslint#73

Based on this doc, it seems like a bad practice: https://angular.io/guide/styleguide#style-05-03

But I figured out now that the files actually can be excluded from the angular config directly. So we don't need the tsconfig. I adapted this now and it works: mgechev/codelyzer#268 (comment)

Closing this PR although I recommend to adapt the project to the style guidelines at some point...

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