Cobra Forums

Starting a new thread regarding ticket 34 so we don't clutter the other one...

hopscc wrote:Sorry hadn't answered - been away last week...

That failing test was to for completeness to exercise a case with all the possible clauses (nicely laid out on separate lines) and with the changed positioning of the initialisation clause - its distinct from the cases with the clauses joined on (a) single line(s)
Its a reasonably logical layout that should be valid...

The coding of the patch is a better approach than the previously more rigorous position dependent probing for clauses in expected orders..
Leave it as is FTM and I'll take a task to look at whats needed to support the failing case...

As of right now with 2014-02-24_var-assign-is-has.patch, the rule for fields is that the modifers/attributes should either preceed the assignment operator on the same line or be indented on subsequent lines.

If you we make the assignment statement valid in an indented block, I think the rule for valid syntax becomes harder to verbalize, document, and implement. But I do the see the merits of saying it should be supported for the sake of consistency. Not sure how often that syntax would actually get used though. Up to you.

Here's a bug in the last patch though: If you type...

It's supposed to give the dev the error message: The "is" keyword and modifiers should either precede the equals sign or be indented on the next line per line 1320 but instead you get Expecting an expression. "shared" is a reserved keyword that is not expected here.. Works correctly for the "has" case. I think this is because

on line 1312 is what throws for the "is" case, so the clause that would display the correct message is never entered. Oops...

"Each clause may be in a separate line indented (in any order),
If a both a modifier clause and initialiser clause are on the same line and they are contiguous, the modifier clause( is ...
must precede the initialiser clause ( = ... )."


I was thinking about this today;
for consistency it would be nice to allow all the clauses to be on separate lines indented
... it shows each item nicely formatted/optionally-ordered/clarity/readability, etc ... aesthetically tho' it looks a little funky
all other clauses have a keyword prefix, initialisation has punctuation char (=) which looks a little strange when combined with the
other clauses (as in the example you gave) -
This is less of an issue if the initialisation clause is continued (trails) on the same line...

I agree with you that the full (sep line each clause indented) syntax is probably unlikely to be widely used ( even if only because all the possible clauses are unlikely to be all widely used together) - OTOH if you have a situation where you do need all or many of them it would be clearer to
allow them to be individually lineated ... funky though it may look
So it will probably come down to how hard it is to implement correctly which was one of the reasons for the OTT tests in the patch ( try to specify each valid possibility and ensure the bad ones were caught....

re 'the bug in the last patch'
That is the same situation as the original problem, initialisation prior to an is clause, the initialisation expression not terminating on the is
cos is is a valid keyword in an (initialisation target) expression... then fails on the 'shared' ID.
Its not a problem with has attributes clause cos that keyword isnt overloaded in an expression so the expression terminates when it hits it...

probably comes back to order dependent if on single line, relaxed order if mult-line indented and (preferably) drop support for the old ordering...

More after I've looked at the code/patches again....

OK - chked out both patches..

The latest one (nerdzeros) is a bit more flexible as to ordering for both oneliner and multiline indented options which i expect is goodness..
The additional error message and checking(on IS-clause) wasnt ever firing cos the error and message happened in a called method before the check...

Anyway - I've put a patch on the ticket that should address all that -It should apply on the current codebase ( i.e nerdzeros applied patch code).
It works for initialiser assignment on both oneliner and multiline and traps the
offending situation lost above and gets the message fired ...
I reasserted all the tests that had been removed and added an extra one for the added IDE order situation and a test for the original reversed clauses and indeterminate "is" situation ... Passes all the test suite...

If you are feeling paranoid feel free to apply it against the IDE build and chk thats its all OK but if the original failure report/sample code against that was the only issue it should be OK....

Patch applied.