-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
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 |
A migration path could also be to generate both folders (i.e. the old one symlinking to the new one). |
@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? |
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:
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. |
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 |
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:
|
Hi @mara-schulke. I have been looking into As pointed out many times by others, this forces one to:
From your comments, it seems that this is actually a feature in your setup. Maybe I misunderstand how you are using |
Buffrs does not allow underscores in the
package.name
field and recommends hyphens as a word separator. On the other hand, the Protobufpackage ...;
directive does not accept hyphens and underscores can be used as a word separator.This leads to inconsistencies between the
package
andimport
directive.This is because Buffrs will construct the
proto/vendor
folder asI 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.The text was updated successfully, but these errors were encountered: