Conversation
bf77291 to
2fee123
Compare
|
This would need to work in m-c too. I don't think it would make sense to have some crates adopt this and not others (ie, this should be a broader policy decision that we'd expect all moz crates to adopt over time). And my personal subjective opinion is that this degree of enforced consistency is a backwards step - sometimes an explicit choice is made about how functions are ordered in a file or how elements are listed in structs for (also subjective) reasons - ie, IMO, "perfect" is in the eye of the beholder. I'd push back strongly if this was proposed for crates I have any decision making ability on. |
That’s fair, and thanks for being direct about it. I think this reads as a bigger proposal than it is though. It’s one component, no tooling, no CI, nothing that changes for any other crate or for m-c. The line in my description about enforcing it in CI was more like an open question. Nothing moz-wide intended. I updated the PR description to better reflect that. On the substance, my rule isn’t “alphabetical always”. It’s alphabetical by default, and any other order is fine when a comment says why. Without that comment the order only lives in the head of whoever wrote it, and on a team of more than one person the next person just appends to the end, so after a while it stops being a system at all. Alphabetical is a good default for the cases where nothing more precise has been stated. I think Prod/Staging/Test could be a good example of an order worth keeping, sorted by importance. And this could be stated in a comment. Happy to close it if you think this is a bad idea though. Before I do, I’d like your read on the rule as I’ve described it: alphabetical by default, with a comment whenever there’s a better order. Is that still a backwards step in your view, or was it mostly the CI and moz-wide part you were reacting to? Useful for me to know either way. |
2fee123 to
a129de3
Compare
Mechanical ordering pass over `components/ads-client`. Alphabetical by default, with a comment wherever a different order is deliberate. No behaviour change: the diff is 216 insertions against 212 deletions, and the four extra lines are the two explanatory comments below. What moved: struct fields, enum variants, trait and impl items, derive lists and struct literals, into one consistent order. Consts, types, constructors, pub fns, then private fns, each alphabetical. Derive lists keep trait hierarchy pairs together rather than splitting them alphabetically, so `PartialEq, Eq` and `PartialOrd, Ord` stay adjacent and in dependency order. Three places keep the order they had: * `EffectiveTtl` lists its fields in resolution priority order to match `resolve()` directly below it. * `Environment` is ordered by importance: the deployed environments, then the test one, then the custom escape hatch. * `MozAdsEnvironment` is the uniffi mirror of `Environment`, and uniffi encodes an enum by declaration index, so its variant order is part of the generated bindings. No `uniffi::Record` field order changes. The reordered structs are all component internal and the reordered error enums are plain `thiserror`. The `Telemetry` reorder is in a local impl block, not in the `#[uniffi::export(callback_interface)]` trait. `fmt`, `clippy --all-targets -D warnings` and the 101 unit tests are green.
a129de3 to
9feecfb
Compare
|
Thanks for the thoughtful response. I wanted to share my opinion as a seed for others to do the same. If your proposal is just for this one crate I defer to your team to make the decision. However, the CI question does seem important - without CI checking it it will certainly go stale, but having CI check it means you will need to be confident it will be fine to ask someone to set up a job which does it in moz-central's taskcluster environment. Ultimately though, I have opinions but am not the decision maker for this crate. |
This branch has not been deployed
2 participants
A draft to see whether this is worth having in
ads-client. Scoped to this one component: no tooling, no CI, and nothing that asks anything of any other crate.The convention is alphabetical by default, with a comment wherever a different order is deliberate. The reason for a default at all is that an intentional order usually goes unwritten, so the next person appends to the end and after a while it stops being a system. Alphabetical is a reasonable fallback for the cases where nothing more precise has been stated.
What moved: struct fields, enum variants, trait and impl items, derive lists and struct literals, into consts, types, constructors, pub fns, then private fns, each alphabetical. Derive lists keep trait hierarchy pairs together rather than splitting them alphabetically, so
PartialEq, EqandPartialOrd, Ordstay adjacent and in dependency order.Three places keep the order they had
Each now carries a note saying why, which is the part the convention actually cares about:
EffectiveTtllists its fields in resolution priority order, matchingresolve()directly below it.Environmentis ordered by importance: the deployed environments, then the test one, then the custom escape hatch.MozAdsEnvironmentis the uniffi mirror ofEnvironment. uniffi encodes an enum by declaration index, so its variant order is part of the generated bindings.No behaviour change
The only lines whose content changes are five derive lists reordered in place and one single-line struct literal. Everything else is movement.
fmt,clippy --all-targets -D warningsand the 101 unit tests are green.Beyond those three, no
uniffi::Recordfield order changes: the reordered structs are all component internal, and the reordered error enums are plainthiserror. TheTelemetryreorder is inside a localimplblock, not the#[uniffi::export(callback_interface)]trait.Cost
22 files of churn, which will conflict with in-flight
ads-clientbranches. That is the main thing to weigh against the consistency, and a fair reason to say no.#7613 is separate and does not depend on this.
Pull Request checklist
fmtandclippy --all-targets -- -D warningsclean, 101 unit tests pass.