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

WIP: Preprocessor indention partial rewrite. #119

Closed
wants to merge 1 commit into from

Conversation

ScatteredRay
Copy link
Contributor

This is the beginning of some fixes to the bugs in #118 so I could get some comments.

Really, this is meant to solve the problem of uneven indention depth. I.E.

identity = ->
    matrix = [1, 0, 0, 0,
              0, 1, 0, 0,
              0, 0, 1, 0,
              0, 0, 0, 1]

It doesn't however solve all problems currently related to getting that to parse however.

* Moved peek, err and observe into preprocessor so it'll be easier to use context, i.e. while generating errors.
* Minor abstractions, pulled out SAFEBLOCK, and it's scan.
* Minor fixes.
* Indention code is now more tollerant of various indention depths.
* Fix for parsing out ')'
* On block ends, automatically kill an indention block if it's internal to the block.
@michaelficarra
Copy link
Owner

@Arelius: Why is it desirable to have something like that parse? I would write it like this:

identity = ->
  matrix = [
    1, 0, 0, 0
    0, 1, 0, 0
    0, 0, 1, 0
    0, 0, 0, 1
  ]

And that compiles just fine.

@ScatteredRay
Copy link
Contributor Author

Because a large amount of existing code is written like that and compiles and parses just fine. Additionally, be they right or wrong, people have different stylistic opinions on how their CoffeeScript should be written, and not supporting existing methods isn't a very good way to win over existing CoffeeScript users.

@ScatteredRay
Copy link
Contributor Author

@michaelficarra I was going to continue to work on this, but it seems that you may be strongly against merging this in even if I do finish it. Can you comment on if this is indeed the case?

@alexgorbatchev
Copy link

👍 existing code must compile

@alexgorbatchev
Copy link

If i'm not mistaken, in #45 it was decided to not support existing CoffeeScript markup... am I right?

@ScatteredRay
Copy link
Contributor Author

@alexgorbatchev That seems to be where michaelficarra left it.

@mlogan
Copy link

mlogan commented Jan 2, 2013

Michael,

Regardless of the outcome of this particular PR, would you please clarify whether backwards compatibility with existing CoffeeScript code is a goal of CoffeeScriptRedux? Your silence on this PR casts doubt on that question.

Thanks,
Mark

@alexgorbatchev
Copy link

👍

@michaelficarra
Copy link
Owner

Sorry for the delayed response, everyone.

@Arelius:

I was going to continue to work on this, but it seems that you may be strongly against merging this in even if I do finish it. Can you comment on if this is indeed the case?

I'm not strongly against it, but it's nowhere near the top of my priority list. It'd be interesting to see a good implementatoin.

@alexgorbatchev:

If i'm not mistaken, in #45 it was decided to not support existing CoffeeScript markup... am I right?

Not at all. It is still a top priority.

@mlogan:

please clarify whether backwards compatibility with existing CoffeeScript code is a goal of CoffeeScriptRedux

It is the most important goal. Unfortunately, defining the language itself is a little bit tricky. There's a grey area where abused bugs meet obscure, discouraged features. I've just been ignoring that area for now while I polish up everything else so that 99% of users are better off. The other 1% that write code like this can wait a little longer until we figure out some consistent, understandable rules that allow these kinds of constructs.

<rant>It would also help if there was more contribution and less moaning. A lot of people are complaining about these minor inconsistencies but don't contribute an exhaustive list of supported/unsupported cases or derive a consistent set of rules that would allow for the construct they desire. When I shoot down the issues as bugs with the old compiler, all I get in response is a "but it works with the original compiler!", not "it's intentional and here's the rules for determining when you see this case".

In this issue, for example, what's the rule? It would have to be some sort of special case, since the leftmost indentation is inconsistent, something identified as a bug in the original compiler. Also, whitespace within array literals is significant to support arrays that contain functions or implicit objects. This is not Python. But I'm willing to see a more well-defined proposal, even though I'm almost positive this should be categorised as a bug. It's just not proper CoffeeScript code.</rant>

Closing until someone posts an actual proposal.

@ScatteredRay
Copy link
Contributor Author

It would also help if there was more contribution and less moaning.

It's a bit insulting for you to post this as a reply to my PR. While It's clearly incomplete, I am here, not "moaning" as you say, but attempting to contribute.

Knowing that I would not be able to fix this in isolation, I presented some changes, and opened a PR, so that I could get comments on how this should be fixed, presenting some of my initial modifications to help guide the discussion. And then instead of starting a discussion on how this best be done. This was left at

Why is it desirable to have something like that parse? I would write it like this:

So, if you want contribution you need to make a decent attempt at communicating with potential contributors.

Now I'm perfectly willing to start over and discuss how this could get solved properly.

Now looking at the bug in the original compiler, My change seems to handle this well, Since it tracks the indent sizes it can then verify that the matching dedent has a matching length. Which is preferable to enforcing that all indentation in a single file has to be the same. This is behavior much more similar to the Python compiler.

So, can we work through the actual issues here rather than just claiming that this is not an "actual proposal" and closing it?

@alexgorbatchev
Copy link

Lets everyone look at this positively. I don't think anyone is trying to hurt anyone. I undersand @michaelficarra desire for the more concrete spec. At the same time I see that writing spec to duplicate existing "bugs" is a pretty weird affair to begin with. The problem here is that this projects aims to be a better compiler for currently existing and pretty popular language. This unfortunately dictates that all existing code must parse (or we have ourselves a fork). It's not a matter of stylistic preferences or how one thinks it is better for format code, it simply is a matter of "does it compile in coffee"... it does? then it must compile in redux.

Perhaps assembling a list of test cases for what's currently not compiling would be a good start. This list could be in itself a spec.

Thought?

@ScatteredRay
Copy link
Contributor Author

I see that writing spec to duplicate existing "bugs" is a pretty weird affair to begin with.

The problem with languages without specs, and a single implementation is that "bugs" are spec. And fixing bugs isn't just fixing bugs, but it's rather changing spec.

Not that changing spec is inherently net-bad but it's definitely bad by default.

it simply is a matter of "does it compile in coffee"... it does? then it must compile in redux.

I don't think we need to go quite to that extreme, I think for instance if it's a bug that manifests in poorly defined circumstances, that are unlikely to be used purposefully in existing code, it may warrant getting fixed. Weird dedent behavior described in this bug is a likely candidate for instance, unless you can argue that some sane people rely on that behavior.

Perhaps assembling a list of test cases for what's currently not compiling would be a good start. This list could be in itself a spec.

That would work as a start for me, I'll begin with:

identity = ->
    matrix = [1, 0, 0, 0,
              0, 1, 0, 0,
              0, 0, 1, 0,
              0, 0, 0, 1]

Which compiles in Coffee, but not Redux. And I'd argue that that's both sane and well-specified code, and even if not the preferred style of @michaelficarra should still be supported in Redux.

@mark-hahn
Copy link

The problem with languages without specs, and a single implementation is
that "bugs" are spec.

That's not true. If the coffeescript (not redux) issues page has listed a
bug as a bug, then it is a bug, not a spec. Everything else is a desired
spec'd feature. Doing any other way makes redux a new fork, which is not
anything anybody wants.

Any time someone gets the idea that a feature in coffeescript should not be
in redux, then a bug should be posted in the coffeescript issues forum to
give the community a chance to comment, especially Jeremy.

On Thu, Jan 3, 2013 at 12:59 PM, Arelius [email protected] wrote:

I see that writing spec to duplicate existing "bugs" is a pretty weird
affair to begin with.

The problem with languages without specs, and a single implementation is
that "bugs" are spec. And fixing bugs isn't just fixing bugs, but it's
rather changing spec.

Not that changing spec is inherently net-bad but it's definitely bad by
default.

it simply is a matter of "does it compile in coffee"... it does? then it
must compile in redux.

I don't think we need to go quite to that extreme, I think for instance
if it's a bug that manifests in poorly defined circumstances, that are
unlikely to be used purposefully in existing code, it may warrant getting
fixed. Weird dedent behavior described in this bughttps://github.com/jashkenas/coffee-script/issues/1275is a likely candidate for instance, unless you can argue that some sane
people rely on that behavior.

Perhaps assembling a list of test cases for what's currently not compiling
would be a good start. This list could be in itself a spec.

That would work as a start for me, I'll begin with:

identity = ->
matrix = [1, 0, 0, 0,
0, 1, 0, 0,
0, 0, 1, 0,
0, 0, 0, 1]

Which compiles in Coffee, but not Redux. And I'd argue that that's both
sane and well-specified code, and even if not the preferred style of
@michaelficarra https://github.com/michaelficarra should still be
supported in Redux.


Reply to this email directly or view it on GitHubhttps://github.com//pull/119#issuecomment-11860508.

@ScatteredRay
Copy link
Contributor Author

@mark-hahn :
That sounds like a reasonable way to view it.

@michaelficarra
Copy link
Owner

@mark-hahn: You've got it exactly right.

@ScatteredRay
Copy link
Contributor Author

@michaelficarra: Then can we finally discuss the problems with this particular issue?

@michaelficarra
Copy link
Owner

@Arelius: Feel free. But please try to be as detailed as possible, describing the properties of the missing feature or the parsing/identification rules for it. As @alexgorbatchev suggested, plenty of examples in all possible contexts would be a good start.

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.

5 participants