Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Disable tabs indent. Fixes #80. #224

Closed
wants to merge 1 commit into from
Closed

Disable tabs indent. Fixes #80. #224

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 25, 2015

#80

@puzrin puzrin changed the title Disable tabs indent. Fixes https://github.com/nodeca/js-yaml/issues/80. Disable tabs indent. Fixes #80. Nov 28, 2015
@dervus
Copy link
Collaborator

dervus commented Nov 28, 2015

I fear you're turning skipSeparationSpace function into a bigger mess than it already is. The point is — JS-YAML strictly follows (at least tries to) formal grammar definition. Not general statements in the spec. Only formal grammar definition. In this situation we must be able to put a comment at every line of code with a reference to grammar rule in the spec. The problem with skipSeparationSpace is (AFAIK) that it tries to combine different but similar rules under one straight-forward block of code. It was a mistake because it seems like I accidentally mixed two distinct rule into one. These rules as I stated in the very first comment on #80 are:

s-l-comments grammar rule is missed, and s-separate is used instead:

skipSeparationSpace(true, -1);

As I also stated there — it's a complicated problem, and I can't just take a look at your solution and say: "Yeah, that's correct". @ingydotnet's words about tabs can't change this either. Yes, I know that tabs are considered as content in YAML, not as indentation. But it is a general statement, and we must follow only grammar rules in the spec. These rules don't say just "Hey, no tabs allowed" as you code assumes if I understood it correctly.

So, what I trying to say is that skipSeparationSpace is messed up in the first place. It implements few rules, and I think nobody can say right now which rules. In this situation I just can't accept any change there without references to rules in the spec. The best solution would be to decompose this function instead to trying to add another hack into it.

By the way, there also should be this test sample:

assert.doesNotThrow(function () { yaml.load('|2\n  \text'); });

@dervus
Copy link
Collaborator

dervus commented Apr 16, 2016

Closed — solution is not correct / inactivity.

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

Successfully merging this pull request may close these issues.

2 participants