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

Deprecate old db #1053

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Deprecate old db #1053

wants to merge 6 commits into from

Conversation

thamara
Copy link
Owner

@thamara thamara commented Jan 8, 2024

Related issue

Closes #935.

Context / Background

The old database format (fixed in 4 entries) is in deprecation since may of 2021, as we move with other features, it's time to remove any mention/handling of such database.

What change is being introduced by this PR?

Removed related code and unnecessary strings.

How will this be tested?

Hopefully with the CI. :)

Copy link

codecov bot commented Jan 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c95578c) 79.36% compared to head (8e8f384) 81.29%.
Report is 14 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1053      +/-   ##
==========================================
+ Coverage   79.36%   81.29%   +1.93%     
==========================================
  Files          19       20       +1     
  Lines        1231     1203      -28     
  Branches      183      176       -7     
==========================================
+ Hits          977      978       +1     
+ Misses        254      225      -29     

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

Copy link
Collaborator

@tupaschoal tupaschoal left a comment

Choose a reason for hiding this comment

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

I didn't fully check if all the removed strings were removed from the translation, but overall seems good to go!

@@ -144,26 +130,12 @@ function importDatabaseFromFile(filename)
}
else
{
assert(entry.type === 'flexible');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we write something on the test that hit this assert?

@@ -6,6 +6,7 @@ import fs from 'fs';

import { validateTime } from './time-math.js';
import { generateKey } from './date-db-formatter.js';
import { assert } from 'console';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm trying to keep all external imports at the top of the file, and local imports below

@@ -6,6 +6,7 @@ import fs from 'fs';

import { validateTime } from './time-math.js';
import { generateKey } from './date-db-formatter.js';
import { assert } from 'console';

/**
* Returns the database (only flexible calendar entries) as an array of:
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe we can also remove the "flexible" comments and name functions now that the other doesn't exist anymore

flexibleEntry.values.splice(index, 0, oldStoreHours);
return flexibleEntry;
}

function importDatabaseFromFile(filename)
{
const flexibleStore = new Store({name: 'flexible-store'});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Likewise this doesn't need to be called flexibleStore anymore
Maybe just store or entryStore

@@ -27,7 +27,7 @@ describe('Validate json', function()
});
});

describe('validate date with and without leading 0', function()
describe('validate date with and without leading 0', function()
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this be in a separate pr/commit?

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.

Deprecate old database references
3 participants