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

Double // on one line won't compile #829

Closed
davidhesselbom opened this Issue Sep 22, 2014 · 32 comments

Comments

Projects
None yet
4 participants
@davidhesselbom
Contributor

davidhesselbom commented Sep 22, 2014

Double-commenting a line causes rock to fail with Expected include, import, statement or declaration, which is annoying when you're trying to debug your code by commenting things out. Can this please be changed?

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Sep 22, 2014

Ok, I'll take a look into nagaqueen right now :)

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

That's because triple-slash is an oocdoc, which is only valid before a
function declaration, variable declaration, type declaration, and a few
other things.

On Mon, Sep 22, 2014 at 2:16 PM, Alexandros Naskos <notifications@github.com

wrote:

Ok, I'll take a look into nagaqueen right now :)


Reply to this email directly or view it on GitHub
#829 (comment).

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Ok, but // // fails with the same error message.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Wait... my mistake, the problem was somewhere else. Sorry!

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

Probably because it allows for whitespace between the slashes. I'm not
saying it's just a legitimate issue, just providing pointers as to what the
cause is :)

On Mon, Sep 22, 2014 at 2:19 PM, David Hesselbom notifications@github.com
wrote:

Ok, but // // fails with the same error message.


Reply to this email directly or view it on GitHub
#829 (comment).

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Argh, ok, // // works, //// does not.

Mondays...

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Sep 22, 2014

I guess it assumes "////" is a doc coment (as in /// + / as first character of the doc comment)

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

@Shamanas Yeah, that's what I've been saying :)

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Sep 22, 2014

Yes, indeed, totally missed that first comment.
So it is not a bug, is it?

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

There's been a few duplicates of this issue in recent memory - the real issue, imho, is that the error message isn't helpful enough: it should say that it's parsed as an oocdoc and that it's an invalid oocdoc location.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

So, basically, #565?

How are oocdocs written properly, anyway?

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

@davidhesselbom precisely.

Well, most SDK classes are well-documented using oocdoc. io/File for example.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

... and, would it make sense to parse any number of consecutive / greater than 3 the same as 2 of them, possibly with a warning like "possibly mistaken doc comment ///"?

@fasterthanlime, ok, but the only occurrence I've seen of that is

/** Text */

I haven't actually seen /// being used anywhere. At any rate, I don't think it would be that much of a problem to require that /// is followed by space (one or more) to be parsed as a doc comment, or at least that if immediately followed by / it's not parsed as a doc comment.

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

Well, /// could be used to describe simple class members. It's just copied from Java, which has both /** */ and ///, to mirror /* */ and //.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Sure, but as far as I can see, /// has not been used yet, so allowing //// to be parsed as a non-doc comment wouldn't break any existing code... in the SDK, at least.

For what it's worth, Java also allows both /// and //// anywhere in the code.

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

No need to start an argument about it! I'm 100% okay with tolerating //// everywhere :) and I'm even considering the hypothesis that maybe erroring on misplaced oocdocs isn't the best idea. So we could just remove that error entirely.

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

(Or it could be demoted to a warning)

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Yeah, I agree. It should be an easy fix to just treat misplaced oocdocs as comments - rock already knows when they're misplaced :)

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

The original point was to let people know when they expected something to be treated as docs, but they were just being ignored instead. So it makes more sense to have that as a warning.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Warnings are cool, for ///. I don't know if I think there should be a warning for ////.

@fasterthanlime

This comment has been minimized.

Collaborator

fasterthanlime commented Sep 22, 2014

Yeah, more than three slashes is just nonsense and should be treated as a regular comment.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 22, 2014

Agree to agree, then. :)

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Sep 23, 2014

Ok, I'll see what I can do right now.
So, from what I gathered:

  • oocdocs now start with exactly three slashes
  • oocdocs can now be placed anywhere, raising only a warning when they are
@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Sep 25, 2014

oocdocs can now be placed anywhere, raising only a warning when they are misplaced

Yeah.

@alexnask

This comment has been minimized.

Collaborator

alexnask commented Sep 25, 2014

I tried to fix that but my Nagaqueen-foo is weak.
Anyway, I'll finish up generic extentions and come back to this one.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 12, 2014

@zhaihj You seem to have a knack for fixing things, maybe you could take a look at this? :)

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Nov 13, 2014

@davidhesselbom
Here's a small patch to make oocdocs only start with "///" and "/**": (Need to be tested though)
zhaihj/nagaqueen@44bd21e

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Nov 14, 2014

Very cool! I haven't managed to figure out how to build Nagaqueen yet, so I can't try it, but I hope I will be able to soon.

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Nov 14, 2014

@davidhesselbom

  1. Build && install greg https://github.com/fasterthanlime/greg
  2. Clone nagaqueen(https://github.com/fasterthanlime/nagaqueen) into rock/../
  3. in ./rock, run "make grammar && make"

In fact I'm considering adding warnings to the misplaced oocdocs but still haven't got enough time...

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Jan 15, 2015

I noticed that cogneco has some commits about ooc-doc but can not find the clone of nagaqueen.... Is that private?

Also, I've tested my patch for a while and everything seems good. Remaining task is adding warnings for the mis-placed ooc-docs. But I can not figure out a smart way to do it.

@davidhesselbom

This comment has been minimized.

Contributor

davidhesselbom commented Jan 15, 2015

Cogneco has no fork of nagaqueen, because @fredrikbryntesson is the only person working on it so far. It's not private, but no changes have been pushed yet: https://github.com/fredrikbryntesson/nagaqueen

@zhaihj

This comment has been minimized.

Contributor

zhaihj commented Jan 15, 2015

Thank you!
I'll wait for updates :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment