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

The requirement to merge the derive attribute was decided almost univocally #141

Open
nagisa opened this issue Jun 17, 2019 · 0 comments
Open

Comments

@nagisa
Copy link
Member

nagisa commented Jun 17, 2019

In this PR #105 a following requirement has been added to which I don’t agree:

There must only be a single derive attribute.

Is an unnecessary absolute which in certain corner cases will result in worse, more concise formatting than would otherwise be possible with multiple derive attributes were allowed. Consider:

-#[derive(Clone, Copy, Debug, PartialEq, PartialOrd)]
-#[derive(serde_derive::Serialize, serde_derive::Deserialize)]
+#[derive(
+    Clone, Copy, Debug, PartialEq, PartialOrd, serde_derive::Serialize, serde_derive::Deserialize,
+)]

Where previous set up has the benefit of grouping along with being concise, the new setup spans more lines, does not immediately reveal any relationship between elements and it only gets worse as more derived traits are added:

-#[derive(Clone, Copy, Debug, PartialEq, PartialOrd)]
-#[derive(serde_derive::Serialize, serde_derive::Deserialize)]
-#[derive(Banana, Peach)]
-#[derive(Who, What, When, Where, Why)]
+#[derive(
+    Clone,
+    Copy,
+    Debug,
+    PartialEq,
+    PartialOrd,
+    serde_derive::Serialize,
+    serde_derive::Deserialize,
+    Banana,
+    Peach,
+    Who,
+    What,
+    When,
+    Where,
+    Why,
+)]

Furthermore, when comments get involved, things get even sketchier:

 // In current implementation this gets merged!
-#[derive(Clone, Copy, Debug, PartialEq, PartialOrd)]
-#[derive(serde_derive::Serialize, serde_derive::Deserialize)]
-#[derive(Banana, Peach)]
-#[derive(Who, What, When, Where, Why)] // Derive the 5Ws
+#[derive(
+    Clone,
+    Copy,
+    Debug,
+    PartialEq,
+    PartialOrd,
+    serde_derive::Serialize,
+    serde_derive::Deserialize,
+    Banana,
+    Peach,
+    Who,
+    What,
+    When,
+    Where,
+    Why,
+)] // Derive the 5Ws

 // But this does not, violating the rule for any reasonable interpretation of MUST or ONE
-#[derive(Clone, Copy, Debug, PartialEq, PartialOrd)]
-#[derive(serde_derive::Serialize, serde_derive::Deserialize)] // Derive serde
-#[derive(Banana, Peach)] // Derive fruits
-#[derive(Who, What, When, Where, Why)]
+#[derive(
+    Clone, Copy, Debug, PartialEq, PartialOrd, serde_derive::Serialize, serde_derive::Deserialize,
+)] // Derive serde
+#[derive(Banana, Peach)] // Derive fruits
+#[derive(Who, What, When, Where, Why)]

There doesn’t appear to be any discussion pertaining this issue (or any discussion at all, really) on this kind of rule that I could find, only this merged PR. Should we have a discussion or remove the requirement from the normative specification?

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

No branches or pull requests

1 participant