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

feature: Add update API for generated specifications #27

Merged
merged 19 commits into from
Nov 3, 2023

Conversation

Chrylo
Copy link
Contributor

@Chrylo Chrylo commented Oct 23, 2023

Very similar to the subscribe logic but calls "update" on every child property. The PR also adds a lot of convenience API for developers who are using the generated specifications.

If some classes could not be processed then these should be skipped
instead of retrying the generation.
These add a lot of convenience to the usage of manipulating values
inside these models. No one has to remember the correct copy method
now.
Comment on lines 78 to 79
DEFAULT_FILE_NAME,
classDeclaration.toClassName().simpleName + FILE_NAME_PROCESSOR_POSTFIX,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you shortly elaborate the purpose of this change?

classDeclaration should be the class which is annotated by the VssDefinition class, so in our case it would be KuksaDataBrokerActivityProcessor...?

Copy link
Contributor Author

@Chrylo Chrylo Oct 24, 2023

Choose a reason for hiding this comment

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

Correct. It eliminates an issue where you could annotate two different classes and kapt will then throw an error.

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood, but will it then also process everything twice, because it found the same annotation (with same or different spec file) twice?
Expectation: That should not work and should in fact throw a meaningful exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the specification has the same information for a model then it will throw an exception. If it does not (an extension file for example) it should work (did not test that one yet). We will probably have to support that in the near future anyway and can test it then and create a ticket if something is missing.

- The plus operator (which replaced a child inside a parent) was now
replaced with the invoke operator. Plus and so on are now properly
adding values together for numbers.
# Conflicts:
#	kuksa-sdk/src/main/kotlin/org/eclipse/kuksa/DataBrokerConnection.kt
- Add the update method for vssSpecifications again. The
DataBrokerConnection from the main was used for the initial fix
of the conflicts.
Copy link
Contributor

@wba2hi wba2hi left a comment

Choose a reason for hiding this comment

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

LGTM

Multiple onPropertyChanged updates from different threads may be
called. The updatedVssSpecification must be in sync however. Calling
the .copy in a blocking context is necessary for this.
The documentation of the newSingleThreadContext recommends the usage
of Dispatcher.limitedParallelism in our case.
Copy link
Contributor

@wba2hi wba2hi left a comment

Choose a reason for hiding this comment

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

LGTM

@wba2hi
Copy link
Contributor

wba2hi commented Nov 3, 2023

@lukasmittag @SebastianSchildt
PR can be merged

Copy link
Contributor

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

lgtm

@SebastianSchildt SebastianSchildt merged commit 0535546 into eclipse-kuksa:main Nov 3, 2023
3 checks passed
@erikbosch erikbosch deleted the feature-23 branch October 31, 2024 13:16
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.

3 participants