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

Renaming & Refactoring #7

Merged
merged 31 commits into from
Jan 30, 2024
Merged

Renaming & Refactoring #7

merged 31 commits into from
Jan 30, 2024

Conversation

siefkenj
Copy link

@siefkenj siefkenj commented Jan 29, 2024

This PR primarily introduces new naming conventions with the goal of making it easier for other developers writing components.

The proposed (and included) changes are:

  • DependencyInstruction -> DataQuery. A DependencyInstruction is called an Instruction because it instructs Core how to get the data it wants. Another name for this is a query. Query sounds more transparent to me because a developer doesn't really care what core does/how it does it. They just want the answer to their question, which is: "what is the data that satisfies these restrictions?"

  • New convention: RequiredData. Suppose we're authoring the Foo state variable. Instead of defining a structure FooDependencies and having FooDependencyInstructions automatically created, a developer creates a private struct RequiredData and RequiredDataGraphQueries is automatically created.

    Motivation: Using a uniform name will make state var files look more similar and will hopefully make them easier to follow, since they will have very similar structures. We also have used the word Dependency for several things. FooDependencies could be interpreted as describing graph queries or as describing the StateVarPointers or as describing the data that has been retrieved by core. Calling it RequiredData makes it clear that it is 1) required for computation, and 2) is data and not pointers/requests.

  • dependency_values -> query_results. This could have other names, like required_data_cache or just required_data. This is set after core executes a GraphQuery, so I named it query_results, but this could be a bad name because query_results could be populated by default values (and used before core has done anything?)

  • StateVarInterface -> StateVarUpdaters. The trait StateVarInterface defined methods used by core to solicit what data a state var needed, and to trigger it to recompute its data. Updaters is more descriptive than Interface.

  • The word Interface has been removed from the General*StateVar structs. It didn't seem to be adding anything.

  • General*StateVar structs were renamed *StateVar structs and put in a general_state_var module.

  • StateVarReadOnlyView -> StateVarView. Rust convention is that everything is immutable by default and mutable things are distinguished by special words. In general a view should be considered immutable, so we can use the shorter name.

BooleanState::get_value_dependency_instructions(),
)
.into(),
value: BooleanStateVar::new_from_children().into(),
Copy link
Owner

Choose a reason for hiding this comment

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

I'm a little worried how BooleanStateVar might be confused with StateVar<bool>. It's interesting how you can BooleanStateVar.into() to get a StateVar<bool>.

I can't decide if this similarity is a good thing so authors just do this naturally, or if it will cause confusion.

Copy link
Author

Choose a reason for hiding this comment

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

The nice thing about Rust is that the types are inescapable, so the type information should always be with the developer. We could import them as general_state_var::BooleanStateVar if we really wanted to emphasize it.

/// the dependencies directly on the the structure (`self`)
/// in a form (presumably typed not with enums) for efficient calculation.
/// Called when data queries for the state variable have been completed.
/// State variables are expected to cache the results of their queries
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe "State variables must cache...", as "expected to" sounds like it might be optional to me.

Copy link
Author

Choose a reason for hiding this comment

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

I guess the types might be hard to figure out, but this bit bothered me...it seems like core should cache the results and send them to the component when they're needed...

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, in the Javascript code, core does the caching. I did this solely for the typing, to avoid unwrapping enums on every cycle. Maybe a silly optimization that we can revisit with profiling.

@@ -606,3 +599,6 @@ pub fn add_dependency_data_impl(_attr: TokenStream, item: TokenStream) -> TokenS
_ => panic!("`add_standard_component_fields` has to be used with structs."),
}
}

#[cfg(test)]
Copy link
Owner

Choose a reason for hiding this comment

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

Is this doing something other than reminding us to write tests?

Copy link
Author

Choose a reason for hiding this comment

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

No, just a reminder. I almost wrote some tests, but then got distracted...

@dqnykamp dqnykamp merged commit e0ab4fc into dqnykamp:request-update Jan 30, 2024
3 checks passed
dqnykamp pushed a commit that referenced this pull request Mar 2, 2024
more renaming of state to props
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.

2 participants