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

Move categories to Json #48

Closed
wants to merge 2 commits into from
Closed

Move categories to Json #48

wants to merge 2 commits into from

Conversation

baruchiro
Copy link
Collaborator

@baruchiro baruchiro commented Jun 2, 2020

Related to #31 but not closing because #31 is also about the UI.

@baruchiro baruchiro changed the title Move categirues to Json Move categories to Json Jun 2, 2020
@baruchiro baruchiro marked this pull request as draft June 2, 2020 15:34
@baruchiro
Copy link
Collaborator Author

I hope it working now. I accidentally found that the yarn start is not working because of the import syntax. So I added basic Github Actions in #49 to do a sanity test before merging.

When you are new in a project, you don't have the confidence to add code without testing.

@baruchiro baruchiro marked this pull request as ready for review June 3, 2020 04:19
@@ -0,0 +1,9 @@
{
"search": {
"אינטרנט": "Internet"
Copy link
Owner

Choose a reason for hiding this comment

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

It would be clearer if the key / value meaning was consistent. So if in exact matches the key is the category and value is the string to match, I would do the same for search

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, the search should be also CategoryName to Array of values.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This structure is better for readability. This is why I'm changing it in the code to be seller to category or search keyword to category because when you editing the JSON, you want to see one category and all its values, but in the code, you handle one value each time, and you want the category of it.

@@ -0,0 +1,21 @@
const { search, exact } = require('./categoriesMap.json');

const descriptionToCategory = Object.keys(exact).reduce((acc, category) => {
Copy link
Owner

Choose a reason for hiding this comment

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

This is classic for unit tests. I think we should start improving our test coverage and this is an easy example

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what you mean, but yes, I thought maybe we should write a test to validate the JSON structure, make sure there are no duplicates.

Copy link
Owner

Choose a reason for hiding this comment

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

What I mean is that this code is self contained and can be tested easily in a unit test.

Given a json categories map, a test for exact matches and a test for search matches. We can discuss offline if my meaning is not clear

@@ -0,0 +1,21 @@
const { search, exact } = require('./categoriesMap.json');
Copy link
Owner

Choose a reason for hiding this comment

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

The reason I had a categoryCalculation-example file and had the actual categoryCalculation file git ignored is that the contents of this file will change per person. Especially since category names are not necessarily the same for everyone.

After this change it will be a problem to have a personal mapping file because it means changing a git tracked file.

So we can either continue with some variation of having an example and real ignored file or we can implement this as a file that the app stores in the file system (like the config or part of the config). The second solution is better but will require ui to allow editing it.

WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. How do you expect the users will use our product? From the source code or from the Release? I'm guessing from both.
  2. For release, we need a ready-to-use file, but now I think we can prepare the source before releasing it.
  3. For running from source, I think we need a few preparations as possible, and it means we need a ready-to-use file in the source.
  4. We already talked about creating a global default categories mapping, and per-user override mapping, I think it will solve the problems, also if the user wants other names to the same categories, we can implement that based on the global mapping.
  5. I need to remember that the require will be bundled with webpack, so this file will not be available for customizing in the Release.

Maybe all these concerns require us to open an issue for decide the whole solution before the development.

Copy link
Owner

Choose a reason for hiding this comment

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

Maybe all these concerns require us to open an issue for decide the whole solution before the development.

Agreed, I think we need some more design on the solution. I will add my thoughts to the issue and we can discuss a bit more before implementing.

To the points you mentioned:

  1. Both
  2. For release as well I expect the user to be able to customize his categories as he pleases which points in the direction of saving another file like the config. This can be a global with overrides but the problem with that is that it could map to categories that I as a user don't even have (in tools like ynab or other structured budgeting tools). The solution may be to have a default mapping but if the result is a category the user doesn't have just ignore it.
  3. Agree that it needs to be seamless from source but that doesn't necesarily mean having a hard coded file in git. It could work just like the config - if there isn't a categories mapping, the code will create the file from the example.
  4. See 2
  5. Yes, thats why we should probably store this as a file in the data directory that will be available also on release and not as a file in the source code.

@baruchiro
Copy link
Collaborator Author

@brafdlog I'm closing this, there is no special work I did here, and we need to think clearly without my work.

@baruchiro baruchiro closed this Mar 21, 2021
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