gdext icon indicating copy to clipboard operation
gdext copied to clipboard

Change "minimal" dependency feature to package feature

Open nikitalita opened this issue 2 years ago • 4 comments

When "minimal" was added as a feature, examples and itest included the godot package with the feature "minimal".

As a result, when building the workspace, even if the workspace is compiled with --no-default-features, it will still only run godot-codegen with the "minimal" feature set. If we change it so that the dependency feature is enabled by a package feature which is on by default, we can then compile the workspace with --no-default-features and get the codegen for the full class set.

nikitalita avatar Nov 07 '22 21:11 nikitalita

A bit of background:

The minimal feature was a bit of a hack to allow shorter dev cycles, as codegen took a huge amount of time for all the classes. I'm not sure if we want to support it long-term, the distinction is also rather arbitrary.

But we can gladly improve the way this is handled until then. Technically, features should be additive (specifying an extra feature should add functionality, not remove it), so I'm doing it wrong. If we went that way, we would have to:

  • instead of minimal, have a counterpart codegen-full, which generates everything when active, and the minimal set when inactive
  • have codegen-full on by default (so people can just use the library without getting errors)
  • let itest and maybe demo use this (untested):
    [dependencies]
    godot = { path = "../../godot", features = ["convenience"], default-features = false }
    

I didn't do it like this so far, because it's less convenient than just specifying minimal. In particular, packages need to list all the features if no-default-features is set, meaning that we have to maintain this "default" list in several places.

However, someone just running cargo build in the workspace would still have the minimal set enabled, even if only one package uses it... So maybe we should change it. Your example on the other hand would require cargo build --no-default-features in the workspace, whereas most people naturally don't add that command line argument.

What was your circumstance to build the workspace but require full codegen? The library itself doesn't need it. Did you want to generate documentation?


Apart from that, maybe only the itest should have a minimal set at all (for faster CI), and the examples should specify dependencies closer to how the user would do it.

Bromeon avatar Nov 07 '22 23:11 Bromeon

I was just dicking around seeing how certain classes that weren't in the minimal set got generated. I wanted to see how you were handling properties on classes that had their default setters and getters as protected methods (it turns out the answer was "not").

nikitalita avatar Nov 07 '22 23:11 nikitalita

Yeah, codegen is not yet feature-complete. Properties, constants, etc. still need to be exposed 🙂

What do you think about having a codegen-full feature instead of minimal? Would that address your issue?

Bromeon avatar Nov 08 '22 09:11 Bromeon

Yeah that would work for me.

nikitalita avatar Nov 08 '22 19:11 nikitalita