EyeofBeholder-NLeSC / orange3-argument

Argument analysis, mining, and visualization add-on for Orange3.
https://research-software-directory.org/software/orange3-argument-add-on
Apache License 2.0
1 stars 1 forks source link

Merge branch dev_processor: refactor and test #58

Closed jiqicn closed 1 year ago

jiqicn commented 1 year ago

This PR is for refactoring the processor module and adding proper units and integration tests.

To review this PR, one can pay attention to the following:

NB: It's a design decision to escape from OOP for this module, and the reason behind is that it's simply not necessary. The new design abandons the unnecessary encapsulation of data within classes while improving testing efficiency while maintaining code readability.

jiqicn commented 1 year ago

Hi @carschno I just forgot to add you as the reviewer of this PR. Now you should have everything ready. Again, many thanks for helping out!

jiqicn commented 1 year ago

I find the changes very clear and well implemented. I put some comments in-place, mostly reflecting personal preferences and optional suggestions.

In general, I

  1. would tend to remove some of the more simple functions and replace them by respective checks where that can be done in a single line/call.
  2. am not sure if the tests using mocks are effective in testing the respective functionality.

I might be missing the motivation for my second point, let's discuss that in person.

Many thanks for the extremely quick reply! I will have to work on something else for the rest of today, but will definitely check all the comments carefully tomorrow, and let's talk on Thursday for all points that you think should be further discussed!

carschno commented 1 year ago

let's talk on Thursday for all points that you think should be further discussed!

I am at a conference on Thursday and Friday, though. So tomorrow (Wednesday) is the only option left this week. Just send me an invite for whenever it suits you (this week or later).

jiqicn commented 1 year ago

let's talk on Thursday for all points that you think should be further discussed!

I am at a conference on Thursday and Friday, though. So tomorrow (Wednesday) is the only option left this week. Just send me an invite for whenever it suits you (this week or later).

Will do!

sonarcloud[bot] commented 1 year ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication