Remove mutable state from Evaluators and some related, poorly tested functions #655
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #655 +/- ##
====================================
+ Coverage 60% 61% +1%
====================================
Files 100 99 -1
Lines 4657 4555 -102
Branches 1106 1080 -26
====================================
- Hits 2811 2787 -24
+ Misses 1379 1302 -77
+ Partials 467 466 -1
|
minichma
force-pushed
the
work/minichma/feature/remove_some_state
branch
from
54573de to
a025e65
Compare
|
We could set |
minichma
force-pushed
the
work/minichma/feature/remove_some_state
branch
from
0949024 to
b33e704
Compare
|
It would certainly be great to make some progress on the nullable front, but I'd also appreciate not to grow the PR mor than necessary and keep concerns separated. |
|
@axunonb What do you think of removing those methods I mentioned. It would certainly be possible to implement them also without the state that was removed, but I struggled with understanding the intended purpose, because partly untested and no sufficient docs. Do you agree that it should be ok to remove them in the first v5 version? Should people complain, we could re-evaluate and re-introduce. |
There is always a certain doubt when changing code without docs and missing unit tests. Sometime there is code that might aim to serve achieving a certain target, but with the full implementation left. |
|
Alright, its just the two tests in TodoTest that I removed, but they also only covered certain execution paths. |
| @@ -13,37 +13,16 @@ namespace Ical.Net.Evaluation; | |||
|
|
|||
| public abstract class Evaluator : IEvaluator | |||
| { | |||
| private DateTime _mEvaluationStartBounds = DateTime.MaxValue; | |||
| private DateTime _mEvaluationEndBounds = DateTime.MinValue; | |||
There was a problem hiding this comment.
Just a sidenote: Initializing with DateTime.MinValue in ical.net is a potential risk because we can't create a DateTimeOffset with it.
There was a problem hiding this comment.
Absolutely agree. So another benefit of them being gone. There are quite many of those magic values throughout the lib. Should implement Nullable in most of those places.
Yep, let's try to pick one file after the other, instead of a big bang. If helpful, include in a PR, otherwise chose a separate PR. |
Yes, I think so. The number of missed lines is reduced. The reduced number of hits originates from code that has been deleted, which basically is good for the code quality. We don't have new issues and I think that most new code is covered. The modified code that isn't covered hasn't been covered before as well and I don't feel in the position to write all those tests in a meaningful way at this time. Most notably the |
Good finding, and I'm sure nobody will even notice that it's gone. Yes, let's remove the class. |
…ng the dependency on `PeriodListEvaluator`. Make related methods private or internal. Avoid using `IEvaluator.Periods` so it can be removed.
…ounds` and `.Periods` properties, because it isn't needed anymore and removing it vastly reduces complexity.
…y on internal state of `Evaluator` that should be removed. They are fully untested, so the intended functionality and usage pattern are rather unclear. Moreover it seems that the functionality can be implemented by the user quite easily and exactly to their requirement.
…not needed anymore.
minichma
force-pushed
the
work/minichma/feature/remove_some_state
branch
from
b33e704 to
64c1847
Compare
|
|
Removed the |
The complexity of occurrence evaluation is impacted by the internal state maintained by classes implementing
IEvaluatorand related. This PR aims to reduce that complexity by eliminating much of this state. Only a few functions rely on this state, and these are either poorly documented, untested, or better suited for user implementation. The benefits of removing this state appear to significantly outweigh the drawbacks.Most notable the following public methods are removed:
ToDo.IsCompleted(): poorly tested, i.e. only basic execution paths are tested, but not the more complex ones.ToDo.IsActive(): seeIsCompleted()CalendarEvent.Occurs*(): untested and they seem to be better suited for user implementation.As a benefit the following internal state can be eliminated:
IEvaluator.PeriodsIEvaluator, i.e..Evaluation*BoundsEvaluator._mEvaluation*Bounds