-
-
Notifications
You must be signed in to change notification settings - Fork 3
Conversation
Arp-G
commented
Jan 25, 2024
Thanks for the PR 🙏 . I will review it soon.
gaurav-kreeti
commented
Feb 8, 2024
Thanks for the PR 🙏 . I will review it soon.
Just a Reminder to review, in case you forgot.
Arp-G
commented
Feb 18, 2024
Hi, thanks for the PR again. There are some issues (some of them not directly related to this PR)
I am listing some of them in order of priority
-
There is some problems with the genstage pipeline sometimes due to which the genstage consumers sit idly and do not make any demand to the producers because of which the csv import just stops. I haven't yet found a solution to this. Just try importing a different number of csvs and you will come across this.
-
The way configs are working right now is messy, I do not want to keep a CLI app anymore and only want to support interaction with the app via the web interface. So modules like the
Csv2sql.Config.Parserare no longer needed. Need to go through the entire config flow and clean it up. -
For this new rewrite of the app I am trying to move away from polling the
Csv2sql.ProgressTrackermodule and instead use a pub sub-based system. Where the live view process can subscribe to the progress tracker and the progress tracker process will send progress updates to the live view process. This might not be a good idea since too many progress update messages between the processes can lead to bad performance so polling might be better, but need to give it a try. -
Some tests are failing and tests for some critical code paths are missing.
Some other issues include
- Need to move away from the usage of shorter_maps and remove it
- Update all deps, there are some warnings when running on latest elixir/erlang, nodejs versions
- Use latest template live view syntax for the pages, example:
caseblocks,<%= if ..calls with the template should be avoided. For case blocks just create a helper function, for conditions use the new:if={..}syntax, for loops use the new:for={...}syntax. - The UI for start page is very basic right now, its a good start but needs lot of improvement. Like disable updating configs when csv import is in progress, disabling the start import button if entered configs are invalid, etc.
I tried to resolve some of the above issues here but was stuck on the first point. Will look into it again when I get a chance. Meanwhile, you can take a look and continue where I left off. I suggest getting an understanding of the core logic of the app.
UI for succesfully parsing: Screencast from 23-01-24 03:22:31 PM IST.webm
if some error occured:
Screencast from 23-01-24 03:27:15 PM IST.webm