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

chore: remove unsed vars & cleanups #437

Merged
merged 1 commit into from
Jan 10, 2023
Merged

chore: remove unsed vars & cleanups #437

merged 1 commit into from
Jan 10, 2023

Conversation

MatissJanis
Copy link
Member

Removing a couple more unused variables + a tiny improvement in the compiler to make some more tests pass.

@netlify
Copy link

netlify bot commented Jan 7, 2023

Deploy Preview for actualbudget ready!

Name Link
🔨 Latest commit 30f0288
🔍 Latest deploy log https://app.netlify.com/sites/actualbudget/deploys/63b9af9a63f0b30008e65a30
😎 Deploy Preview https://deploy-preview-437--actualbudget.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

let [arg1] = valArray(state, args, ['string']);
return typed(`LOWER(${arg1})`, 'string');
}

// integer/float functions
case '$neg': {
validateArgLength(args, 1);
// eslint-disable-next-line no-unused-vars
let [arg1] = valArray(state, args, ['float']);
valArray(state, args, ['float']);
Copy link
Member Author

Choose a reason for hiding this comment

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

Throws an exception if the vars are invalid, so we can't remove it.

// });
// });
// });
test('querying transactions works', async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Only uncommenting these tests. No changes in them.

@@ -183,7 +183,7 @@ export default class Spreadsheet {
this.events.emit('change', { names: this.computeQueue });

// Cache the updated cells
if (this.saveCache) {
if (typeof this.saveCache === 'function') {
Copy link
Member Author

Choose a reason for hiding this comment

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

Making the tests pass.

@MatissJanis MatissJanis marked this pull request as ready for review January 7, 2023 17:49
@TechwizEE
Copy link

Performed the following steps and verified the test cases pass

  • Separately cloned master and matiss/unused-vars
  • Ran command yarn test:node spreadsheet.test.js in folder actual/packages/loot-core
  • Saw the two un-commented tests were passing
  • Total passing testcases: 4 of 4

Performed the following steps to verify the code change to spreadsheet.js line 186

  • In file spreadsheet.js, reverted line 186 to master (i.e. if (typeof this.saveCache === 'function') { to if (this.saveCache) {)
  • Re-ran test, saw test case querying deep join works fail with TypeError: this.saveCache is not a function
  • Reverted to matiss/unused-vars, re-ran test again, saw querying deep join works pass

Please let me know if there are additional steps/actions I can take to assist in resolving this issue.

@trevdor trevdor merged commit 89dc139 into master Jan 10, 2023
@trevdor trevdor deleted the matiss/unused-vars branch January 10, 2023 01:34
@trevdor
Copy link
Contributor

trevdor commented Jan 10, 2023

Thanks for reviewing, @TechwizEE!

eshaffer321 pushed a commit to eshaffer321/actual-fork that referenced this pull request Jan 19, 2023
FlorianLang06 pushed a commit to FlorianLang06/actual that referenced this pull request Mar 7, 2024
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