Implemented missing Journal constructor so Initialize method is called. #310
Conversation
|
Can you get rid of Initialize(), and move that logic to the constructor? And then implement the change in net-core as well? |
- Prevented multiple passes while extrapolating - Extended setter logic, so that duration is adjusted when end changes (and vice versa)
|
Initialize(): Initialize() is also called in OnDeserializing. However atm Initialize() is a one-liner (Name = Components.Journal;). Would you still prefer to move the logic to both (constructor and OnDeserializing) or keep it in the existing Initialize() method? Or just remove from OnDeserializing as this should not be necessary? net-core: no problem! I will adjust it then too. |
I think it's safe to move it to the constructor. It's just setting properties, and by rights, this is a constant property for the type. In fact, it could probably be real |
This reverts commit 2971e35. Reason: Not intended for current pull request.
…" object when instantiated. - Removed redundant OnDeserializing().
|
Moved Initialize() logic to constructor and removed reduntant OnDeserializing() in |
There was a problem hiding this comment.
Does this solve a problem you have? I.e., do you need me to publish a nuget package version that incorporates these change?
If so, can you:
- Update the
readme.mdfile, and explain quickly what problem was solved with these changes. - Bump the minor version in the respective nuspec files in
v2andnet-core?
v2/ical.NET/Components/Event.cs
Outdated
| else if (Duration == default(TimeSpan) && DtStart != null && DtEnd != null) | ||
| { | ||
| Duration = DtEnd.Subtract(DtStart); | ||
| if (propertyName == nameof(DtEnd)) |
There was a problem hiding this comment.
Please use braces for conditionals.
v2/ical.NET/Components/Event.cs
Outdated
| } | ||
| else if (DtStart == null && Duration != default(TimeSpan) && DtEnd != null) | ||
| { | ||
| DtStart = DtEnd.Subtract(Duration); | ||
| } | ||
| else if (/*Duration == default(TimeSpan) && */ DtStart != null && DtEnd != null) |
There was a problem hiding this comment.
Please don't commit commented-out code.
v2/ical.NET/Components/Event.cs
Outdated
| /// <summary> | ||
| /// Flag to check if ExtrapolaTimes is currently invoking to prevent multi extrapolating | ||
| /// </summary> | ||
| private bool _isExtrapolating; |
There was a problem hiding this comment.
Does this solve a problem that you have a test case for?
There was a problem hiding this comment.
I will open another Pull Request incl. description to separate this from the current request.
v2/ical.NET/Components/Journal.cs
Outdated
| { | ||
| Name = Components.Journal; | ||
| } | ||
|
|
||
| protected override bool EvaluationIncludesReferenceDate => true; | ||
|
|
||
| protected override void OnDeserializing(StreamingContext context) |
There was a problem hiding this comment.
I seem to recall that deleting these overrides can cause subtle problems, so I think these should be put back.
I'm sorry my other comment was unclear: I meant delete the call to Initialize() from this method, moving that logic to the constructor, which you've done. Can you put back this override? Then it will be ready to merge.
There was a problem hiding this comment.
Reverted removal of OnDeserializing()
(see 7a3f471)
If not intended, the constructor of the Journal class was missing and its Initialize() Method was not called which led to "Component: null" object when instantiated.