Skip to content
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

Buffrs enforces inconsistencies between package directive and import paths #230

Open
NiklasRosenstein opened this issue Mar 22, 2024 · 8 comments · May be fixed by #236
Open

Buffrs enforces inconsistencies between package directive and import paths #230

NiklasRosenstein opened this issue Mar 22, 2024 · 8 comments · May be fixed by #236
Labels
complexity::medium Issues or ideas which require some discussion, research and more implementation work component::cli Everything related to the buffrs cli datamodel Changes related to the Datamodel priority::low Please dont work on this if you can contribute something with a higher priority type::refactoring Changing the inner-workings of buffrs type::style Related to personal or community opinions

Comments

@NiklasRosenstein
Copy link

Buffrs does not allow underscores in the package.name field and recommends hyphens as a word separator. On the other hand, the Protobuf package ...; directive does not accept hyphens and underscores can be used as a word separator.

This leads to inconsistencies between the package and import directive.

# Proto.toml
[package]
name = "foo-bar"
type = "api"
version = "0.1.0"
// proto/service.proto
package foo_bar;
import "foo-bar/messages.proto";

This is because Buffrs will construct the proto/vendor folder as

proto/vendor/
  foo-bar/
    service.proto
    messages.proto

I would argue that this is inconsistent and confusing for no benefit and that Buffrs should instead align with the Protobuf specification; if not for the Buffrs package.name, at least for the generated directory structure.

@mara-schulke
Copy link
Contributor

FWIW this would be a huge breaking change to everyone using buffrs at the moment (e.g. require manual intervention in every single project)

@Tpt
Copy link
Contributor

Tpt commented Mar 22, 2024

FWIW this would be a huge breaking change to everyone using buffrs at the moment (e.g. require manual intervention in every single project)

Can't this be gated on the edition? If the package foo_bar of any package depending on it is using edition = 0.8, then proto/vendor/foo-bar/ is generated and the package foo_bar of any package depending on it is using edition = 0.9 then proto/vendor/foo_bar/ is generated? This way all edition = 0.8 packages can use the import foo-bar syntax and all edition = 0.9 packages the import foo_bar syntax. This might lead to some duplication in the vendor directory during the migration but it shouldn't be a big deal

@NiklasRosenstein
Copy link
Author

A migration path could also be to generate both folders (i.e. the old one symlinking to the new one).

@mara-schulke
Copy link
Contributor

mara-schulke commented Mar 25, 2024

@Tpt yes, but I dont see huge value in this change other than it being a style issue at the moment?

It works reliably and doesn't cause any issues hence im hesitant on spending time on this as it also would break existing projects (one would need to migrate all proto file imports manually, even with editions).

@NiklasRosenstein what is the motivation behind this? Is it related to python support?

@mara-schulke mara-schulke added datamodel Changes related to the Datamodel type::style Related to personal or community opinions component::cli Everything related to the buffrs cli complexity::medium Issues or ideas which require some discussion, research and more implementation work type::refactoring Changing the inner-workings of buffrs priority::low Please dont work on this if you can contribute something with a higher priority labels Mar 25, 2024
@heatonmatthew
Copy link

heatonmatthew commented Mar 25, 2024

For an additional perspective, I've introduced Buffrs at my company and this inconsistency was something I needed to document well for my teams in our usage guidelines (as it wasn't intuitive). We basically just accepted it as "how things were done" for this tool.

If it were to change, my perspective would be to suggest:

  • Support both hyphen (-) and underscore (_) in the Proto.toml package name
  • Be completely deterministic regarding Proto.toml package name and the name of the path in the vendor folder (i.e. don't switch in a version and do some sort of internal conversion of - to _)
  • Then users can choose when to rename the package in the Proto.toml file to match the *.proto package name (and take the hit to remedy any consuming proto files to update their import statements.

That seems like it would provide an opt-in migration path. But there might be other restrictions in how the Proto.toml file is consumed which means it can't accept underscores in the package name.

@kixa
Copy link

kixa commented Mar 25, 2024

FYI no underscore support breaks our current legacy generate-and-package JS (via: https://github.com/protobufjs/protobuf.js) setup too since the generated code uses relative imports and expects package ... to match the folder name. I.e. Given (for example) package my_proto (in the proto), package.name: my-proto (in proto.toml) and generating stubs from vendor/ after install, the output will contain (for example) import * from ../my_proto, but since the folder name is my-proto, every import fails.

@poliorcetics
Copy link
Contributor

We could introduce a change in the form of:

[package]
name = "foo-bar" # Current version, would stay unchanged
directory = "foo_bar" # Optional, new, overrides `name` if set

That way project that don't have the issue would just continue working and those that do have the escape hatch

Somewhat prior art:

@dgehriger
Copy link

Hi @mara-schulke. I have been looking into buffrs and it really looks very promising. However, I'm also suffering from the fact that the package statements don't allow dashes, while buffrs Proto.toml files require them.

As pointed out many times by others, this forces one to:

  • name a package in Proto.toml as e.g. name = api-examples-hub
  • name the same package in the .proto file as package api_examples_hub
  • import that package as import "api-examples-hub/foo.proto"
  • refer to its types as api_examples_hub.Type

From your comments, it seems that this is actually a feature in your setup. Maybe I misunderstand how you are using buffrs in your organization. My suspicion is that you aren't using hierarchical names. Is that the case? If not, why this restriction? Simply allowing . in the Proto.toml name would solve the problem, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
complexity::medium Issues or ideas which require some discussion, research and more implementation work component::cli Everything related to the buffrs cli datamodel Changes related to the Datamodel priority::low Please dont work on this if you can contribute something with a higher priority type::refactoring Changing the inner-workings of buffrs type::style Related to personal or community opinions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants