search

Hypertopic / LaSuli

Social annotation for qualitative analysis
https://hypertopic.org/lasuli
GNU General Public License v3.0
12 stars 4 forks source link

Notifications on errors when modifying #63

Closed franck-eyraud closed 5 years ago

franck-eyraud commented 6 years ago

Get the error given by the db library and transform it in information for the user. Did that on the 2 current modification requests : #47 and #52

Currently using the notification system of Firefox, not sure if it is the most obvious to see for a user, could be changed.

But most of the modifications are in the function flow, as getting the error needs to be coherent in the way functions are called (synchronous vs asynchronous). This pull request proposes to use mainly synchronous method, and Promises, and avoid async and await keywords. If OK, we could generalize to all legacy code.

franck-eyraud commented 6 years ago

This is how notification currently appears screenshot from 2018-11-19 10-18-44

franck-eyraud commented 6 years ago

Rebased to new master (v3) branch.

franck-eyraud commented 6 years ago

Changed tabulations into spaces

benel commented 6 years ago

This pull request proposes to use mainly synchronous method, and Promises, and avoid async and await keywords.

Using promisses is indeed the nicest way to go (as proposed in Hypertopic/lib-node documentation). It fosters a very declarative way, both concise and elegant, and is very well suited for the kind of error handling we want (empty list to process instead of an error).

benel commented 5 years ago

Dear @franck-eyraud,

Both pull requests #63 and #65 are opened, but #63 seems to include #65, whereas it was opened earlier; and #63 has conflicts whereas #65 has not.

I am not sure about the one to merge...

franck-eyraud commented 5 years ago

Pull #63 changes things in many places so easily creates conflicts. This is why I rebased #63 on top of #65, to propose the resolution with #65 changes.

The original branch changed further, so other conflicts would need to be solved again. Did you test this pull request #63 which adds more notifications to the user ? If you consider that it is worth merging, I will resolve the conflicts with branch v3.

In the meantime, you can merge #65 if you want.

benel commented 5 years ago

For your information, I also implemented a new feature (fd96d5224f46d566f027ee433313911818590c20). I haven't pushed it yet on master since you have the priority (your code was there first) ;)

franck-eyraud commented 5 years ago

OK. Can you try the last push I sent ? it solves the conflict that there was with this pull request.

Or maybe do you prefer that I merge also your new feature ?

benel commented 5 years ago

Replaced by PR #75 to make integration step by step (commit by commit).