Skip to content

Typed attribute api#4352

Open
timon-schelling wants to merge 3 commits into
masterfrom
typed-attr-api
Open

Typed attribute api#4352
timon-schelling wants to merge 3 commits into
masterfrom
typed-attr-api

Conversation

@timon-schelling

Copy link
Copy Markdown
Member

No description provided.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 41 files

Confidence score: 4/5

  • node-graph/nodes/text/src/lib.rs is tied to a PR-title policy violation rather than a code defect, so the main merge risk is process friction (failing checks or delayed approval) rather than runtime behavior—rename the PR title to imperative mood before merging.
  • node-graph/libraries/core-types/src/list.rs now boxes/downcasts f64 defaults via implicit_default when opacity is missing, which could add avoidable per-item overhead in graphics-heavy paths if merged as-is—move the default onto Attr (or use a typed non-allocating default path) to de-risk performance.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/nodes/text/src/lib.rs">

<violation number="1" location="node-graph/nodes/text/src/lib.rs:1">
P2: Custom agent: **PR title enforcement**

The PR title "Typed attribute api" does not follow the imperative mood requirement: "Typed" is past tense, but commit titles must always be written as a command/instruction using an imperative verb like "Add", "Fix", or "Implement". Additionally, the standard abbreviation "API" is written in lowercase as "api", which violates the project's convention for standard abbreviations (the rule lists "API" alongside "UI", "SVG", "CSS", etc. as abbreviations that should appear in their standard uppercase form).

To fix both issues, the title should use an imperative verb such as "Implement", "Add", or "Introduce", and the acronym should be uppercase. For example: "Implement typed attribute API", "Add typed attribute API", or "Introduce typed attribute API".</violation>
</file>

<file name="node-graph/libraries/core-types/src/list.rs">

<violation number="1" location="node-graph/libraries/core-types/src/list.rs:76">
P3: Rendering graphics without explicit opacity now boxes and downcasts two `f64` defaults per item through `implicit_default`. Put the default on `Attr` (or use a typed non-allocating default path) so missing attributes do not add per-item heap work.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

@@ -1,3 +1,4 @@
pub mod attr;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: Custom agent: PR title enforcement

The PR title "Typed attribute api" does not follow the imperative mood requirement: "Typed" is past tense, but commit titles must always be written as a command/instruction using an imperative verb like "Add", "Fix", or "Implement". Additionally, the standard abbreviation "API" is written in lowercase as "api", which violates the project's convention for standard abbreviations (the rule lists "API" alongside "UI", "SVG", "CSS", etc. as abbreviations that should appear in their standard uppercase form).

To fix both issues, the title should use an imperative verb such as "Implement", "Add", or "Introduce", and the acronym should be uppercase. For example: "Implement typed attribute API", "Add typed attribute API", or "Introduce typed attribute API".

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/nodes/text/src/lib.rs, line 1:

<comment>The PR title "Typed attribute api" does not follow the imperative mood requirement: "Typed" is past tense, but commit titles must always be written as a command/instruction using an imperative verb like "Add", "Fix", or "Implement". Additionally, the standard abbreviation "API" is written in lowercase as "api", which violates the project's convention for standard abbreviations (the rule lists "API" alongside "UI", "SVG", "CSS", etc. as abbreviations that should appear in their standard uppercase form).

To fix both issues, the title should use an imperative verb such as "Implement", "Add", or "Introduce", and the acronym should be uppercase. For example: "Implement typed attribute API", "Add typed attribute API", or "Introduce typed attribute API".</comment>

<file context>
@@ -1,3 +1,4 @@
+pub mod attr;
 pub mod fallback;
 mod font;
</file context>

/// The value an item without attribute `A` is considered to have: the key's implicit default if it
/// has one, otherwise the value type's `Default`.
fn implicit_default<A: Attr>() -> A::Value {
implicit_default_value(A::name())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: Rendering graphics without explicit opacity now boxes and downcasts two f64 defaults per item through implicit_default. Put the default on Attr (or use a typed non-allocating default path) so missing attributes do not add per-item heap work.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/libraries/core-types/src/list.rs, line 76:

<comment>Rendering graphics without explicit opacity now boxes and downcasts two `f64` defaults per item through `implicit_default`. Put the default on `Attr` (or use a typed non-allocating default path) so missing attributes do not add per-item heap work.</comment>

<file context>
@@ -132,14 +60,24 @@ unsafe impl<T: StaticTypeSized> StaticType for Bundle<T> {
+/// The value an item without attribute `A` is considered to have: the key's implicit default if it
+/// has one, otherwise the value type's `Default`.
+fn implicit_default<A: Attr>() -> A::Value {
+	implicit_default_value(A::name())
+		.and_then(|value| value.into_any().downcast::<A::Value>().ok())
+		.map_or_else(Default::default, |value| *value)
</file context>

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

Successfully merging this pull request may close these issues.

1 participant