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

refactor(core): use private variables instead of function wrappers #1106

Merged
merged 2 commits into from
Oct 10, 2023

Conversation

JKRhb
Copy link
Member

@JKRhb JKRhb commented Oct 3, 2023

As another addition to #1104, this PR replaces a couple of internal getters (that have been used for avoiding serialization) with built-in private variables, making the code a bit cleaner.

@JKRhb JKRhb force-pushed the internal-getters branch from 2ea520b to 3ec46f7 Compare October 3, 2023 19:05
@JKRhb JKRhb force-pushed the internal-getters branch from 3ec46f7 to ff5ffe5 Compare October 3, 2023 19:08
@codecov
Copy link

codecov bot commented Oct 3, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (b51ef19) 75.52% compared to head (fcc3b70) 75.56%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1106      +/-   ##
==========================================
+ Coverage   75.52%   75.56%   +0.03%     
==========================================
  Files          80       80              
  Lines       16153    16154       +1     
  Branches     1520     1513       -7     
==========================================
+ Hits        12200    12207       +7     
+ Misses       3914     3908       -6     
  Partials       39       39              
Files Coverage Δ
packages/core/src/consumed-thing.ts 83.94% <100.00%> (+0.57%) ⬆️
packages/core/src/exposed-thing.ts 67.58% <75.00%> (+0.42%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JKRhb JKRhb marked this pull request as ready for review October 3, 2023 19:11
Copy link
Member

@danielpeintner danielpeintner left a comment

Choose a reason for hiding this comment

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

LGTM

Maybe it makes sense to add a comment why we use # but in this case I don't think I would do that since we would need to explain the rational all over the place.

I think it is good to go as is 👍

@JKRhb
Copy link
Member Author

JKRhb commented Oct 9, 2023

I tried to address your feedback, @danielpeintner, in fcc3b70, let me know if you see further room for improvement :)

Copy link
Member

@relu91 relu91 left a comment

Choose a reason for hiding this comment

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

were they included in the TD? I've never noticed, otherwise, the comment is a little bit misleading.

@relu91 relu91 merged commit 9557fe1 into eclipse-thingweb:master Oct 10, 2023
7 checks passed
@JKRhb JKRhb deleted the internal-getters branch October 10, 2023 20:07
@JKRhb
Copy link
Member Author

JKRhb commented Oct 10, 2023

were they included in the TD? I've never noticed, otherwise, the comment is a little bit misleading.

The result is basically the same as before, the only difference is that we do not need the getter construction anymore. We could also remove the comments as it is common JS behavior, I guess it depends on how useful the reminder of this behavior is.

@danielpeintner
Copy link
Member

Note: We used to have the workaround to wrap the internal state into functions to not be stringified in a TD. The current approach using private variables is much better in my regard. Hence I think the comment is fine 🤷‍♂️

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