Skip to content

Conversation

@WasabiFan
Copy link
Member

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

@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 😉

@ddemidov
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

Done.

@WasabiFan
Copy link
Member Author

Thanks! You're fast!

@ddemidov
Copy link
Member

This looks good to me, so we probably need a 👍 from @rhempel.

@WasabiFan
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

The README looks decent.

Yes! Decent! The quality bar I strive for 😉

. 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
Copy link
Member Author

@rhempel Can you take a look at this?

@rhempel
Copy link
Member

rhempel commented Dec 29, 2015

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
Copy link
Member

rhempel commented Dec 29, 2015

But overall yes - 👍

@WasabiFan
Copy link
Member Author

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
Copy link
Member

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

@ddemidov
Copy link
Member

ddemidov commented Jan 1, 2016

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

WasabiFan added a commit that referenced this pull request Jan 1, 2016
Modify autogen for decentralized repo model
@WasabiFan WasabiFan merged commit 8f4b4e6 into develop Jan 1, 2016
@WasabiFan WasabiFan deleted the decentralized-repo-model branch January 3, 2016 07:08
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.

4 participants