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

#340 merging pi4j-board-info into pi4j-core #341

Merged
merged 5 commits into from
Apr 8, 2024
Merged

#340 merging pi4j-board-info into pi4j-core #341

merged 5 commits into from
Apr 8, 2024

Conversation

FDelporte
Copy link
Member

To be able to use the data from the model in a REST API, this requires an extra dependency. Is this a problem?

com.fasterxml.jackson.core:jackson-annotations

@FDelporte FDelporte requested a review from eitch April 4, 2024 15:43
Copy link
Member

@eitch eitch left a comment

Choose a reason for hiding this comment

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

We should perform the code to detect the board only once, and then store it in a singleton, or a class field on the BoardInfo class. It's quite heavy work to keep doing it, and it can't change at runtime anyhow.

@eitch
Copy link
Member

eitch commented Apr 5, 2024

To be able to use the data from the model in a REST API, this requires an extra dependency. Is this a problem?

com.fasterxml.jackson.core:jackson-annotations

Shouldn't we keep the JSON stuff out of core Pi4j? Let the REST API code generate the JSON code, then we don't need this dependency.

@FDelporte
Copy link
Member Author

Shouldn't we keep the JSON stuff out of core Pi4j?

@eitch I was expecting this ;-) is removed now...

@FDelporte
Copy link
Member Author

We should perform the code to detect the board only once, and then store it in a singleton.

It's called from DefaultContext.java, so it's part of a singleton, correct?

https://github.com/Pi4J/pi4j-v2/pull/341/files#diff-34f18b9362bf258d05dac12071fa50bf49f05a9459c7875dbbb2044bdbe23962R107

@eitch
Copy link
Member

eitch commented Apr 5, 2024

We should perform the code to detect the board only once, and then store it in a singleton.

It's called from DefaultContext.java, so it's part of a singleton, correct?

https://github.com/Pi4J/pi4j-v2/pull/341/files#diff-34f18b9362bf258d05dac12071fa50bf49f05a9459c7875dbbb2044bdbe23962R107

Sure, but maybe someone else would call BoardInfo.current(), as it is a static helper method.

@FDelporte
Copy link
Member Author

Sure, but maybe someone else would call BoardInfo.current(), as it is a static helper method.

yep, in that case, it's up to the user to decide if they want to do that multiple times, right?

@eitch
Copy link
Member

eitch commented Apr 6, 2024

Sure, but maybe someone else would call BoardInfo.current(), as it is a static helper method.

yep, in that case, it's up to the user to decide if they want to do that multiple times, right?

True, but from a library i expect it to avoid unnecessary computations. So multiple calls should cache this computation somehow.

@eitch eitch merged commit ddc57ba into develop Apr 8, 2024
1 check passed
@eitch eitch deleted the feature/#340 branch April 8, 2024 06:47
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