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

feat(diagnostics): adds invalid casing diagnostic #16

Merged
merged 1 commit into from
Nov 3, 2016

Conversation

eamodio
Copy link
Contributor

@eamodio eamodio commented Oct 24, 2016

This is a simple first pass at providing diagnostics

@CLAassistant
Copy link

CLAassistant commented Oct 24, 2016

CLA assistant check
All committers have signed the CLA.

@eriklieben
Copy link
Contributor

Wonderful thanks!

I tried to get the code and run it, when should this activate? On typing something like

<input value.Bind="foo">

Or in some other case?

Do you know if this overlaps with the checks provided in https://github.com/MeirionHughes/aurelia-template-lint? It's on my list to implement the aurelia linting in the vscode extention.

@eamodio
Copy link
Contributor Author

eamodio commented Oct 24, 2016

@eriklieben Yeah -- it will give errors if you do something like <custom-element onFooChange.bind="foo"> rather than <custom-element on-foo-change.bind="foo">

I was also trying to lay the ground work to be able to validate that both sides of the expression exist (within reason). But I would need to use the typescript language service for that I think.

Hrm -- I hadn't hear of that linter -- I will definitely check that out to see if there is overlap, or better to use the linter directly in the language server to provide errors.

I just desperately wanted the above error reported, since I (and others) have lost SO much time for forgetting to kebab-case things in the HTML. :)

@MeirionHughes
Copy link

MeirionHughes commented Oct 24, 2016

@eamodio I'm currently in the middle of reworking the linter and the long term goal is to have it used by the vscode addon to provide language and context information. I think I have a check somewhere for camelCase attribute names values. If it needs tweeking for your needs feel free to open an issue with your user case.

Ideally, the vscode addon will leverage aurelia-template-lint for "issues" detected in the html and source templates.

@eamodio
Copy link
Contributor Author

eamodio commented Oct 24, 2016

@MeirionHughes so it sounds like I shouldn't play with getting the linter into the language server then? Wait for the rework?

@MeirionHughes
Copy link

MeirionHughes commented Oct 24, 2016

That is up to @eriklieben really. It will overlap, eventually, just not right now. So it probably makes sense to have this until the linter can take over.

Also the linter runs off a html5 parser (parse5). So in this specific case, the linter actually cannot detect the camelCase because it drops the case down. I could investigate the raw stream to determine any camelCase names, if need be.

The rework is still a few weeks away; but the major addition will be that is will tell you if the custom-element has the bindable you're trying to use, plus whether the target (if you use typescript) is assignable to the bindable.

@eriklieben
Copy link
Contributor

Excellent, I will have some time tomorrow evening to review and merge it, to include it in the main branch. Let add it for now and when we have integrated the lint tool check if there is overlapping functionality at that moment in time and what to do about that.

Just as a side note: The linter can already validate the expression part (not sure if you mean that with both sides of the expression), so I don't know if it's worth the time to invest time in building out the expression validation part. Might be that once that code is done, it close to the timeframe when we integrate the linter.

@eamodio
Copy link
Contributor Author

eamodio commented Oct 28, 2016

@eriklieben great!

Yeah, I was thinking about that exact functionality -- so very happy it already exists :)

@eriklieben
Copy link
Contributor

eriklieben commented Oct 31, 2016

Hi, sorry for the late response I was sick for a few days and didn't sit behind a PC that much.

I tried to merge/ resolve the merge conflicts, but I can't push to the branch in the pull request.

Could you try to do a merge from master to your branch and resolve the conflicts? Then I can probably merge it from the pull request to the master branch; this is only allowed when merge requests are resolved.

If I try to merge it to a custom branch and resolve the conflicts and squash it again, I am afraid I lose your name as the author of the work.

Thanks!

@eamodio eamodio force-pushed the feature-diagnostics branch from f256157 to daa3b36 Compare November 3, 2016 03:15
@eamodio
Copy link
Contributor Author

eamodio commented Nov 3, 2016

@eriklieben sorry for the delay, but I've rebased the branch and resolved the conflicts. Hopefully it is good to go now.

Aside -- what should be used for spacing? It looks like a mix of tabs and spaces and wasn't sure which it should be.

@eriklieben eriklieben merged commit eee039c into aurelia:master Nov 3, 2016
@eriklieben
Copy link
Contributor

No problem, thanks!

2 spaces according to the .editconfig, I will streamline it over the weekend, also need some tslint errors to fix. Just been into fix mode to resolve the autocomplete for 1.6.1 and didn't pay much attention to lint errors for a bit :-)

@eriklieben
Copy link
Contributor

It's released to the marketplace :-)

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.

4 participants