-
Notifications
You must be signed in to change notification settings - Fork 47
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
Conversation
I hope it working now. I accidentally found that the When you are new in a project, you don't have the confidence to add code without testing. |
@@ -0,0 +1,9 @@ | |||
{ | |||
"search": { | |||
"אינטרנט": "Internet" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- How do you expect the users will use our product? From the source code or from the Release? I'm guessing from both.
- For release, we need a ready-to-use file, but now I think we can prepare the source before releasing it.
- 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.
- 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.
- I need to remember that the
require
will be bundled withwebpack
, 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.
There was a problem hiding this comment.
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:
- Both
- 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.
- 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.
- See 2
- 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.
@brafdlog I'm closing this, there is no special work I did here, and we need to think clearly without my work. |
Related to #31 but not closing because #31 is also about the UI.