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

Removed no longer needed hack #2216

Closed
wants to merge 1 commit into from
Closed

Removed no longer needed hack #2216

wants to merge 1 commit into from

Conversation

@Cangit
Copy link
Contributor

@Cangit Cangit commented Jun 6, 2014

The hack done here: https://github.com/meteor/meteor/blob/devel/packages/less/plugin/compile-less.js#L47
now backfires on my system and is reporting a line number one less than the actual error. I don't know when this behaviour has changed, but I have tested this in multiple scenarios.

(I also see that the less library package is a bit outdated.)

@Slava
Copy link
Member

@Slava Slava commented Jun 6, 2014

Hi @Cangit. Can you please list the examples you test this change on?

@Cangit
Copy link
Contributor Author

@Cangit Cangit commented Jun 9, 2014

This is an example in its purest form
https://github.com/Cangit/LessErrorExample

Error should be reported to line 5, but error message reads like this:
lib/style.less:4:1: Less compiler error: Unrecognised input

@@ -44,7 +44,7 @@ Plugin.registerSourceHandler("less", function (compileStep) {
compileStep.error({
message: "Less compiler error: " + e.message,
sourcePath: e.filename || compileStep.inputPath,
line: e.line - 1, // dunno why, but it matches
line: e.line,
column: e.column + 1

This comment has been minimized.

@Cangit
Copy link
Contributor Author

@Cangit Cangit commented Jun 9, 2014

@mquandalle Yes, the +1 to column is still needed.

@Cangit
Copy link
Contributor Author

@Cangit Cangit commented Jun 10, 2014

I figured out when this behaviour changed, in less 1.6 released january 1st 2014. ref: https://github.com/less/less.js/blob/master/CHANGELOG.md#160

mquandalle referenced this pull request Jun 10, 2014
XXX review the changelog
@Slava
Copy link
Member

@Slava Slava commented Jun 10, 2014

Thanks @Cangit, the reference to the Changelog really helped to understand the issue. I merged it at dbca690.

@Slava Slava closed this Jun 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants