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 StorageHelper to be clearer in its constructor #2417

Merged
merged 3 commits into from
Apr 15, 2021

Conversation

Nokel81
Copy link
Collaborator

@Nokel81 Nokel81 commented Mar 31, 2021

  • Don't create a box and then override it

  • Initial value should be the default value

Signed-off-by: Sebastian Malton sebastian@malton.name

@ixrock This is a simpler form of the fix I think.

@Nokel81 Nokel81 added the bug Something isn't working label Mar 31, 2021
@Nokel81 Nokel81 added this to the 4.2.1 milestone Mar 31, 2021
@Nokel81 Nokel81 requested review from ixrock and a team March 31, 2021 12:23
@Nokel81 Nokel81 self-assigned this Mar 31, 2021
src/renderer/utils/storageHelper.ts Outdated Show resolved Hide resolved
src/renderer/utils/storageHelper.ts Outdated Show resolved Hide resolved
@ixrock ixrock requested a review from a team March 31, 2021 12:37
@aleksfront
Copy link
Contributor

@Nokel81 Can you please post a brief description why this change needed? There's no link to issue or reference to a bug.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 31, 2021

@aleksfront This is trying to fix master where a test is failing on linux.

@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 31, 2021

So after even more debugging I have come to the conclusion that even though I think this is a worth while refactor, the actual solution is to change src/renderer/utils/storageHelper.ts:132 to be return this.data.get() ?? this.defaultValue;.

@Nokel81 Nokel81 changed the title Fix StorageHelp to initialize only once Refactor StorageHelper to be clearer in its constructor Mar 31, 2021
@Nokel81
Copy link
Collaborator Author

Nokel81 commented Mar 31, 2021

Changed this PR into a refactor, and opened #2421 to actually fix the bug.

@Nokel81 Nokel81 added chore and removed bug Something isn't working labels Mar 31, 2021
Copy link
Member

@ixrock ixrock left a comment

Choose a reason for hiding this comment

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

LGTM

@Nokel81 Nokel81 requested a review from jim-docker March 31, 2021 16:24
jim-docker
jim-docker previously approved these changes Mar 31, 2021
src/renderer/utils/storageHelper.ts Outdated Show resolved Hide resolved
@Nokel81
Copy link
Collaborator Author

Nokel81 commented Apr 1, 2021

Will look into this thanks.

@Nokel81 Nokel81 changed the base branch from master to release/v4.2 April 6, 2021 14:29
- Don't create a box and then override it

- Initial value should be the default value

Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
Signed-off-by: Sebastian Malton <sebastian@malton.name>
@Nokel81 Nokel81 force-pushed the initialize-storage-with-default branch from 71ad8f6 to 8fafcb4 Compare April 6, 2021 14:29
@Nokel81 Nokel81 added the merge/master This PR will need to be merged to master after merged to target. Using cherry-pick. label Apr 6, 2021
@Nokel81 Nokel81 modified the milestones: 4.2.1, 4.2.2 Apr 9, 2021
@Nokel81 Nokel81 merged commit da69ffd into release/v4.2 Apr 15, 2021
@Nokel81 Nokel81 deleted the initialize-storage-with-default branch April 15, 2021 19:37
@Nokel81 Nokel81 mentioned this pull request Apr 15, 2021
@Nokel81 Nokel81 removed the merge/master This PR will need to be merged to master after merged to target. Using cherry-pick. label Apr 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants