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

Add end position to warnings and errors #1250

Merged
merged 5 commits into from
Mar 18, 2018

Conversation

jamesbirtles
Copy link
Contributor

Been working on a language server for svelte for better editor support and for that, the end position is needed to nicely display where the errors/warnings are.

I left the start as loc to avoid breaking anyone

Sneak peek

#1235

@codecov-io
Copy link

codecov-io commented Mar 17, 2018

Codecov Report

Merging #1250 into master will not change coverage.
The diff coverage is 89.09%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1250   +/-   ##
======================================
  Coverage    91.9%   91.9%           
======================================
  Files         126     126           
  Lines        4556    4556           
  Branches     1486    1485    -1     
======================================
  Hits         4187    4187           
  Misses        153     153           
  Partials      216     216
Impacted Files Coverage Δ
src/validate/js/propValidators/onteardown.ts 100% <ø> (ø) ⬆️
src/validate/js/propValidators/immutable.ts 0% <ø> (ø) ⬆️
src/validate/js/propValidators/onrender.ts 100% <ø> (ø) ⬆️
src/validate/js/propValidators/events.ts 100% <ø> (ø) ⬆️
src/validate/js/propValidators/computed.ts 100% <ø> (ø) ⬆️
src/validate/js/propValidators/oncreate.ts 100% <ø> (ø) ⬆️
src/index.ts 86.2% <ø> (ø) ⬆️
src/validate/js/propValidators/helpers.ts 100% <ø> (ø) ⬆️
src/validate/html/validateWindow.ts 87.5% <ø> (ø) ⬆️
src/validate/js/propValidators/props.ts 0% <ø> (ø) ⬆️
... and 23 more

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 b3fa965...8902417. Read the comment docs.

@Rich-Harris
Copy link
Member

Awesome! Language server sounds amazing, will fill a big gap in the ecosystem.

Does TypeScript complain if you just pass the whole node through? Am thinking that

-validator.error(`:global(...) must be the first element in a compound selector`, { start: selector.start, end: selector.end });
+validator.error(`:global(...) must be the first element in a compound selector`, selector);

would be a nice shorthand, and communicates the intent of the code nicely.

@jamesbirtles
Copy link
Contributor Author

Ooh yeah that does work, will change

@jamesbirtles
Copy link
Contributor Author

Changed that, I've also added Stylesheet as an export as it's needed to use validate

@Rich-Harris
Copy link
Member

I've also added Stylesheet as an export as it's needed to use validate

Not sure I follow — do you mean in terms of typings? If so I wonder if we should expose the interfaces somehow, rather than making Stylesheet part of the public API.

@jamesbirtles
Copy link
Contributor Author

The validate method, which is exported, takes a stylesheet as a parameter, so in order to call this function you need access to the Stylesheet class

e.g. in compile

const stylesheet = new Stylesheet(source, parsed, options.filename, options.cascade !== false, options.dev);

validate(parsed, source, stylesheet, options);

@Rich-Harris Rich-Harris merged commit 5b086df into sveltejs:master Mar 18, 2018
@Rich-Harris
Copy link
Member

Ah, of course. That's actually slightly unfortunate now that I think about it — makes me think that perhaps validate shouldn't be exposed, but rather called from compile, and any warnings should be attached to the compiled output in the form of a stats.warnings object or something. In fact I had some related thoughts about stats recently that I'll put into an issue for v2.

Thanks for doing this! Look forward to hearing more about the language server — let me know if there's anything I can do to help.

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