ev3dev / ev3dev-lang

(deprecated) language bindings for ev3dev sensors, motors, LEDs, etc.
GNU General Public License v2.0
56 stars 39 forks source link

Modify autogen for decentralized repo model #142

Closed WasabiFan closed 8 years ago

WasabiFan commented 8 years ago

This is a first attempt at the changes discussed in #135.

I think this is completely functional, so I'm looking for feedback on the changes that were made and ideas for improving/modifying the new structure.

I haven't yet updated the README for autogen; I will remove this message when I do.

@ddemidov @rhempel

ddemidov commented 8 years ago

I've created a separate repository for C++ bindings with the decentralized ev3dev-lang as a submodule. It works!

So, two things:

  1. You can remove cpp subfolder as part of this PR.
  2. After that, does it make sense to move everything from autogen folder up a level? So that the command for generation would look like node ev3dev-lang/autogen.js instead of node ev3dev-lang/autogen/autogen.js?
WasabiFan commented 8 years ago

After that, does it make sense to move everything from autogen folder up a level?

Because the primary function of ev3dev-lang is to hold the spec, having code at the top level seems like it could be confusing for some people. But I agree that the extra path qualification is annoying; when I get the chance I'll move theautogen.js file (which is the app entry point) up to the root level and have it reference the autogen folder; that way it's easy to type but it doesn't add a bunch of clutter to the top-level directory.

Also, as @dlech pointed out in #135, once we are comfortable with this structure and have used it for a bit I can move autogen into its own repo and make it an npm module. That way, you can just install the utility globally once and simply type the one- or two-word command name, without any path.

WasabiFan commented 8 years ago

@ddemidov I just added an exec-autogen.js file in the root of the repo that runs the autogen script. I included (what I hope is) the proper shebang so that it can be run more easily on the unices. I just guessed though, so it would be great if someone can test it at some point :wink:

ddemidov commented 8 years ago

Just tested, and it works! Can you make it executable though? Or I could add the commit to the PR myself if your OS does not allow it.

WasabiFan commented 8 years ago

Can you make it executable though? Or I could add the commit to the PR myself if your OS does not allow it.

Assuming you mean "can you chmod +x exec-autogen.js", I'm on Windows so I won't be able to do it. Feel free to amend my PR branch though!

ddemidov commented 8 years ago

Done.

WasabiFan commented 8 years ago

Thanks! You're fast!

ddemidov commented 8 years ago

This looks good to me, so we probably need a :+1: from @rhempel.

WasabiFan commented 8 years ago

I just updated the readme in this branch -- can you take a look? The text hadn't been updated in a very long time, so I'm hoping this is more relevant.

ddemidov commented 8 years ago

The README looks decent.

One other thing I noticed is the definitions of autogen comments for various languages here. It seems we would need to update it in this repo for new languages. Is there a way to move this info into each language's autogen-config.json?

WasabiFan commented 8 years ago

The README looks decent.

Yes! Decent! The quality bar I strive for :wink:

. Is there a way to move this info into each language's autogen-config.json ?

I debated this, but as I see it there are three major problems with doing it this way:

  1. If we change the autogen system to use a different core parser, we would need to change all the users of the system -- depending on how many libraries we have, this could get ugly.
  2. We would need to document the required regex and direct library maintainers to said documentation
  3. We would need to copy-and-paste the "C-style" format to at least 3 places, maybe more, and maintain it separately. We could make the C version the default (and just have JS, Java and C# leave it blank), but I am worried that it could get confusing if some have it and others don't, and this solution only works for one comment style.

Obviously, none of those would prevent us from doing it this way; but it is a tradeoff that we have to consider.

WasabiFan commented 8 years ago

@rhempel Can you take a look at this?

rhempel commented 8 years ago

My instinct tells me to leave the autogen comment strings where they are and to NOT distribute them into each language's autogen-config.json - for the exact reasons that @WasabiFan mentions.

This also leaves the door open for each language binding maintainer to use whatever method they choose to autogen the bindings - the thread that holds everything together is the spec.json file.

At worst, every time we add a new language binding we need to add a new comment regex in one place - that won't happen often enough to be a problem.

rhempel commented 8 years ago

But overall yes - :+1:

WasabiFan commented 8 years ago

It sounds like we're in agreement that this is what we want in the repo -- should I merge it now, or would you like some time to get Python transitioned to using this system (create the config file and add the submodule)?

ddemidov commented 8 years ago

I've created rhempel/ev3dev-lang-python#101 that does the switching for python.

ddemidov commented 8 years ago

I think this can be merged now that rhempel/ev3dev-lang-python#101 is merged.