Created attachment 353825 [details] [review]
Allowed empty namespaces patch.

Allowed empty namespaces patch.

Created attachment 353828 [details] [review]
Tests for empty namespaces.

Here are tests for empty namespaces.
To run them, please use "run_genie_tests.py" file from "genie-improved" branch from here:
https://github.com/vlad1777d/Genie/tree/genie-improved

Also this changes can be cherry-picked from "empty_namespaces" branch:
https://github.com/vlad1777d/Genie/tree/empty_namespaces
, which is build on the basis of lastest "master" branch.
(see last 2 commith)

Review of attachment 353825 [details] [review]:

Basics
------
Patch applies and follows Vala coding conventions.
Patch should:
 - have name of author, rather than their nick name
 - state it is a single patch, rather than a series of patches
 - follow the convention of having the component name prefixed to the subject line, e.g. "genie: Allow empty namespaces", this will then appear in the commit log

Approach
--------
The patch make `pass` a declaration. Previously the following code:

[indent=4]
init
    pass

pass

Would produce an error "error: syntax error, expected declaration  but got `pass' with previous `dedent'".

The purpose of the patch should be to allow a namespace without any members. I was expecting something in parse_declarations(), rather than parse_declaration(). For clarity parse_declarations() should be renamed parse_members(). Note that the function also parses members in the root namespace: see parse_file().

In Vala a function without an empty body is declared with empty braces:

void empty_function() {}

Maybe in Genie a function with an empty body could have been declared as:

empty_function()

but the Genie syntax is to use the `pass` keyword:

empty_function()
    pass

This makes it explicit that the developer intended the function to do nothing.
The main use case I can think of for this are place holders when starting to code a project.

So applying that pattern to namespaces, classes, structs and interfaces I would expect the syntax to be:

namespace EmptyNamespaceExample
    pass

The patch does achieve that syntax. I would, however, argue that `pass` in this context should be seen as a no member declaration, rather than an ignored member declaration. So it should be a syntax error if no members are declared, but a member is also declared. For example the following:

[indent=4]
namespace Test
    def test()
        pass
    pass

will currently produce the error "syntax error, expected declaration  but got `pass' with previous `dedent'" This patch, however, allows this to compile. The advantage of having an error is it reminds the developer that `pass` is no longer a place holder and should be removed because there are now declarations.

Likewise I would expect the following:

[indent=4]
namespace Test
    pass
    def test()
        pass

to give an error. Something along the lines of "expected dedent or end of file, but got `def' after `pass'
The patch allows the above example to compile.

These are the reasons I think the patch needs more work.

The review is larger, than my patch =)
Yes, I'll correct it.


But I must disagree about "`pass` in this context should be seen as a no member declaration, rather than an ignored member declaration.", because:

-The word "pass" itself means passing, and not that objects are absent. Changing it's meaning can confuse users.

"pass" from current patch just does nothing, just like in valaparser.vala, when there are empty scopes. It adds nothing to class and saves it.
-In Python keyword "pass" does the same. It's just passed. So such expression:
def test1():
    pass
    def test2():
        pass
        pass
will not give any errors. It's enough obvious.

-Also I don't see any case, when it'll harm to somebody, if he'll leave pass. It's harmless. And if there would be thrown error, compilation will be breaked, making author swearing and searching that file, where he forgot to remove "pass".




About question, why I changed "parse_declaration" and "parse_namespace_member" instead of "parse_declarations" :

-because except "pass" keyword, there could be something other, so if I would break in parse_declarations if "pass" met, than other will not be compiled.

-because there are also structs and you didn't created bugreport for empty structs. So I would need to write much of logic (if "pass" was met while parent - struct - to throw an error). It's not very difficult, but adding to other "cons" - it's not very good.

-because all that function do only one type of work:
--"parse_declarations ()" handles indent\dedent, launching corresponding to parent "parse_*_member" functions, handles errors (it doesn't analise tokens);
--"parse_namespace_member ()" launches "parse_declaration" and according to it's returning type, adds member to parent, or throws an error.
--"parse_declaration ()" launches corresponding to met keyword "parse_*" functions.

(So adding to "parse_declarations ()" analysis of tokens - not consistent to it work.
Now token is analyzed in function, which analyses keywords and launches other "parse_*" functions. So, as after keyword "pass" there's nothing to parse - we don't launch next function.
"parse_namespace_member ()" obtains type of token (if pass - it's "UnresolvedSymbol" and nothing is done))

Of course, if to force meaning "pass" only to empty namespaces\classes\interfaces, I could rewrite it, but then consistence of meaning of "pass" will be loosed. 
In one case it would mean just to pass this statement, in other - that this class\namespace\interface is empty.




So because of this factors I decided to write as it is. If you disagree, we can discuss it more.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/vala/issues/567.