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

Add some documentation about IDerivedStateComputer #55

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tfonda-fbk
Copy link
Contributor

@tfonda-fbk tfonda-fbk commented Aug 2, 2024

Hi,

Lately I have found this interface quite useful in the project I am working on, but it was not trivial to, first of all, learn of its existence, and then figure out how exactly to implement it. Thus, I thought of adding a dedicated section to the online documentation.

Still, there are a few things I am not sure about, so I would appreciate some feedback about the following:

  • the section title;
  • the usage of the preLinkingPhase parameter of installDerivedState(). Both in this example and in my project I've simply ignored it. I am unsure what could be a scenario in which considering it is a hard requirement;
  • the provided discardDerivedState() implementation is very naive and does not check if an Entity's name has been manually set by the user to that of the overridden Entity plus the suffix. Preserving the name field is not strictly necessary in this case, but it would probably be more elegant to introduce a collection as a class member field, add all Entities processed by installDerivedState() to this collection, and then have discardDerivedState() only operate on the Entities in the collection.

Of course, any other feedback is welcome!
Thanks.

@tfonda-fbk
Copy link
Contributor Author

I've just noticed I did not mention anything about the need for binding the custom IDerivedStateComputer implementation via the DSL's RuntimeModule. I'll amend my commit in the next few days.

@tfonda-fbk
Copy link
Contributor Author

I've added a note about binding.


@Override
public void installDerivedState(DerivedStateAwareResource resource, boolean preLinkingPhase) {
resource.getAllContents().forEachRemaining(eObject -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this example is especially beginner-friendly. The derived state is installed before the linking and here we rely on linking information to compute a name property, which in itself would be necessary to make the names that are indexed. Can you imagine a different use-case that you'd want to show here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@szarnekow
Thanks for the feedback. I've provided this example because it is very similar to what I am doing with my custom IDerivedStateComputer implementation in the project I'm working on. It seems to "just work" as expected. Are you saying that my example, apart from not being beginner-friendly, is built on wrong assumptions? This would mean that I have to rework both the example and, most importantly, the code in my project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's what I fear is the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I will think of another example use case for the documentation. Though first I need to figure out how to address this issue inside my project. I already have sketched up an alternative approach that does not rely on indexing/linking-related information, although I will probably need some help to refine it. But this is off-topic here, I'll open a thread in the Discussions section. I hope you won't mind me pinging you there. Thanks a lot for your help!

@tfonda-fbk
Copy link
Contributor Author

Hi @szarnekow
I've updated my proposed documentation section. Please let me know if this example is more suitable. Thanks!

@tfonda-fbk
Copy link
Contributor Author

Gentle bump :)
I have at least one more documentation improvement proposal, but I don't want to flood the repo with too many PRs at once...

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