Skip to content

Issue #232 - Fix board caching#233

Merged
blaise17 merged 5 commits intodevelopfrom
clare/issue#232/Fix_board_caching
Jun 10, 2025
Merged

Issue #232 - Fix board caching#233
blaise17 merged 5 commits intodevelopfrom
clare/issue#232/Fix_board_caching

Conversation

@blaise17
Copy link
Contributor

@blaise17 blaise17 commented Jun 7, 2025

The ButtonFacade is now no longer a facade...
Other options include:

  1. Not extending Button and changing the speechbar component to explicitly use ButtonFacade (still not really a facade, but no copying)
  2. Putting it back to pre typescript 4 state and ignoring the typescipt warning for the declaration of ButtonFacade
  3. Making the deserialisation work with both underscored and non-underscored properties (prefering non-underscored where present)
  4. A magic, much better, different approach that someone suggests

@blaise17 blaise17 requested a review from psi77 June 7, 2025 16:34
@github-actions
Copy link

github-actions bot commented Jun 7, 2025

Test Results

    1 files  ±0      1 suites  ±0   1s ⏱️ ±0s
134 tests +1  134 ✔️ +1  0 💤 ±0  0 ±0 
135 runs  +1  135 ✔️ +1  0 💤 ±0  0 ±0 

Results for commit a7eb145. ± Comparison against base commit 506c819.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jun 7, 2025

Code Coverage Results

Code Coverage

Package Line Rate Branch Rate Complexity Health
src 100% 100% 0
src.app 93% 85% 0
src.app.button-page 89% 74% 0
src.app.config-page 45% 0% 0
src.app.error-page 92% 100% 0
src.app.main-page 75% 100% 0
src.app.obf-button 43% 0% 0
src.app.obfpage 100% 100% 0
src.app.progress 88% 100% 0
src.app.services.board 71% 79% 0
src.app.services.config 57% 50% 0
src.app.services.error 83% 25% 0
src.app.services.page-stack 71% 100% 0
src.app.services.progress 100% 100% 0
src.app.services.scanning 96% 100% 0
src.app.services.speechbar 97% 88% 0
src.app.speechbar 75% 52% 0
src.environments 100% 100% 0
src.test-utils 100% 100% 0
Summary 82% (702 / 855) 77% (147 / 190) 0

psi77
psi77 previously approved these changes Jun 10, 2025
Copy link
Contributor

@psi77 psi77 left a comment

Choose a reason for hiding this comment

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

I think if this fixes the bug we should merge it as is and then raise a separate ticket for looking at the button facade and refactoring it into something more appropriate

🚀

const testBoard = new OBFBoard().deserialize(testBoardJSON);
boardSet.setBoard('test', testBoard);

service.retrieve().subscribe({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this want to fail? or just warn?
Failure means the test can never get cleaned up because we don't then run the delete.
Warning means the test can never end up deleting real data that has a key clash (may be irrelevant due to domain scoping?)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think failing is legit

@blaise17 blaise17 requested a review from psi77 June 10, 2025 09:35
@blaise17
Copy link
Contributor Author

I think if this fixes the bug we should merge it as is and then raise a separate ticket for looking at the button facade and refactoring it into something more appropriate

🚀

I've rasied #234 to address the facade refactor.

@blaise17 blaise17 merged commit 10c8911 into develop Jun 10, 2025
3 checks passed
@blaise17 blaise17 deleted the clare/issue#232/Fix_board_caching branch June 10, 2025 10:00
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