ADD : Period & ITimeBlock depuis Lucca #63

nfaugout-lucca · 2018-11-02T10:59:08Z

Récupération de ITimeBlock de Lucca, avec une approche légèrement différente. Plutôt que d'avoir toutes les propriétés de Period en get;set; on passe par des méthodes explicites pour changer les propriétés.
Ceci permet à une classe de mettre de la logique métier dans ces méthodes, plus élégant je trouve que de mettre ça dans les setters.
Ca permet aussi à une classe qui refuse qu'on lui change ses valeurs (immutabilité) de lancer des exceptions (ailleurs que dans le setter du coup)

Implémentation de l'égalité entre 2 Periods.

Quelques règles :

une Period ne peut pas être négative (end strictement plus petit que start), ça pète des tests sur le monolithe qui autorisait cette situation
la Duration peut ne pas être égale à end-start, notamment dans le cas d'UD avec une période correspond à une journée et une duration plus longue

Remplacement de TimeInitials par une Culture, et reprise des traductions des initiales d'après ce qui est dans le monolithe

PS : PR à suivre sur Lucca pour intégrer tout ça

sonarqubecloud

SonarQube analysis found issues:
Bugs: 0
Vulnerabilities: 0
Code Smells: 2

See all issues in SonarCloud

codecov · 2018-11-02T11:04:55Z

Codecov Report

Merging #63 into master will increase coverage by 3%.
The diff coverage is 55.37%.

@@           Coverage Diff            @@
##           master      #63    +/-   ##
========================================
+ Coverage   32.19%   35.19%    +3%     
========================================
  Files          30       32     +2     
  Lines        1662     1895   +233     
  Branches      186      193     +7     
========================================
+ Hits          535      667   +132     
- Misses       1105     1203    +98     
- Partials       22       25     +3

Impacted Files	Coverage Δ
...xtends/Primitives/TimeSpans/TimeSpan.extensions.cs	`58.82% <15.38%> (-6.05%)`	⬇️
.../Primitives/DateTimes/NegativeDurationException.cs	`50% <50%> (ø)`
NExtends/Primitives/DateTimes/Period.cs	`56.52% <56.81%> (-29.2%)`	⬇️
NExtends/Primitives/DateTimes/Date.cs	`57.39% <57.39%> (ø)`
...xtends/Primitives/DateTimes/DateTime.extensions.cs	`54.97% <60%> (+0.64%)`	⬆️
NExtends/Primitives/TimeSpans/TimeInitials.cs	`83.33% <66.66%> (-16.67%)`	⬇️
NExtends/Primitives/Strings/String.extensions.cs	`28.43% <0%> (+1.47%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3353e5b...cb618ee. Read the comment docs.

Récupération de ITimeBlock de Lucca, avec une approche légèrement différente. Plutôt que d'avoir toutes les propriétés de Period en get;set; on passe par des méthodes explicites pour changer les propriétés. Ceci permet à une classe de mettre de la logique métier dans ces méthodes, plus élégant je trouve que de mettre ça dans les setters. Ca permet aussi à une classe qui refuse qu'on lui change ses valeurs (immutabilité) de lancer des exceptions (ailleurs que dans le setter du coup) Implémentation de l'égalité entre 2 Periods. Quelques règles : - une Period ne peut pas être négative (end strictement plus petit que start), ça pète des tests sur le monolithe qui autorisait cette situation - la Duration peut ne pas être égale à end-start, notamment dans le cas d'UD avec une période correspond à une journée et une duration plus longue Remplacement de TimeInitials par une Culture, et reprise des traductions des initiales d'après ce qui est dans le monolithe PS : PR à suivre sur Lucca pour intégrer tout ça

sonarqubecloud

SonarQube analysis found issues:
Bugs: 0
Vulnerabilities: 0
Code Smells: 2

See all issues in SonarCloud

rducom

Je te propose plutôt de laisser struct Period struct et immutable, de lui implémenter IEquatable, et de proposer des méthodes d’extensions retournant une nouvelle valeur de struct Period

rducom · 2018-11-02T11:44:43Z

NExtends/Primitives/DateTimes/Period.cs

+
+            return Equals(obj as Period);
+        }
+        public override int GetHashCode()


Il est interdit d'implémenter GetHashCode() sur une une instance mutable, car la plupart des primitives de liste gardent le hashcode calculé en cache. Hors si l'instance est mutable, alors une fois modifiée, le HashCode n'est plus valide, mais il ne sera pas pour autant calculé.
C'est le genre d'effet de bord à se taper la tête contre les murs lors du débug.
D'autre part, pour des class, toujours implémenter un IEqualityComparer<T> plutôt que IEquatable<T>.

Implémentation du type Date qui correspond à une Date sans "time". Une Period est instanciée soit avec des DateTime soit avec des Date, dans les 2 cas elle sait exposer ses start/end en date (startsOn) ou en temps (startsAt). Pour l'instant aucun projet n'utilite la version Date, mais ça pourra venir Start/End tout court sont dépréciés car pas assez explicites, il faut utiliser StartsAt ou StartsOn désormais Un ITimeBlock doit renvoyer un ITimeBlock à chaque fois qu'on modifier ses propriétés - pour une classe mutable, l'objet se renverra lui-même - pour un struct comme Period, on renvoie une nouvelle instance à chaque fois Ca permet de gérer la transition class/struct en douceur côté Lucca sans pour autant devoir faire des refactos trops massifs NB : toute la partie sur l'égalité disparaît puisque Period redevient un struct.

sonarqubecloud

SonarQube analysis found issues:
Bugs: 1
Vulnerabilities: 0
Code Smells: 5

See all issues in SonarCloud

ADD Tests

nfaugout-lucca · 2018-11-03T22:02:27Z

@rducom je suis arrivé à une version "compatible" avec le monolithe, c'était pas gagné ;)

Je te laisse regarder.

rducom

tu introduit la classe Date mais sans s'en servir ailleurs. Est-ce vraiment utile ?
côté monolithe, il y a aussi le struct DateInterval qui overlap avec Period. Ça vaudrait le coup de regrouper

rducom · 2018-11-05T07:34:29Z

NExtends/Primitives/DateTimes/Date.cs

+
+        public static Date operator -(Date d, TimeSpan t)
+        {
+            return new Date(d._dt - t);


Ici : possibilité de créer un objet Date avec minutes / sec / ms. Contrat rompu

Non, car le constructeur en question fait une division euclidienne et ne garde donc que la partie entière du TimeSpan.

Mais ce serait peut-être plus logique en effet de ne prendre que le .TotalDays du TimeSpan.

Après c'est une lib externe donc j'ai pas voulu faire de refacto, déjà que j'ai fixé les retours de Sonar..

rducom · 2018-11-05T07:34:38Z

NExtends/Primitives/DateTimes/Date.cs

+
+        public static Date operator +(Date d, TimeSpan t)
+        {
+            return new Date(d._dt + t);


rducom · 2018-11-05T07:36:55Z

NExtends/NExtends.csproj

@@ -3,11 +3,12 @@
  <PropertyGroup>
    <TargetFrameworks>netstandard2.0</TargetFrameworks>
    <ProjectGuid>{6319D598-FF7C-4B48-8C46-53F1E87C8220}</ProjectGuid>
+    <PackageVersion>2.3.7</PackageVersion>


pas besoin c'est la CI qui gère

Oui c'est ça un oubli de revert dans de commit, je l'enlèverai plus tard

rducom · 2018-11-05T07:37:09Z

NExtends/NExtends.csproj

  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Newtonsoft.Json" Version="11.0.2" />
-    <PackageReference Include="System.ComponentModel.Annotations" Version="4.5.0" />
+    <PackageReference Include="System.ComponentModel.Annotations" Version="4.4.1" />


parce que le monolithe est en 4.4.1 et que j'ai pas envie de le passer en 4.5.0 (breaking changes potentiels) tout de suite.

ou alors je vais faire une PR à part sur le monolithe pour le passer en 4.5.0...

rducom · 2018-11-05T07:38:56Z

NExtends/Primitives/DateTimes/Period.cs

-        public TimeSpan Duration { get { return End - Start; } }
+        public DateTime StartsAt { get; }
+        public DateTime EndsAt { get; }
+        public TimeSpan Duration { get; }


Si Duration n'est plus calculé, ça veut dire qu'on doit à chaque fois s'assurer de la cohérence des données, en amont, je pense pas que ce soit une bonne chose

Regarde les tests, y'en a un qui garanti que Duration "peut" être différent de End-Start, c'est dans le cas particulier d'une période de date à date avec une Duration qui "déborde". Y'a des cas dans le monolithe.

On pourra revenir dessus plus tard via une Issue, mais faudra déjà fixer le "pb" dans le monolithe.

rducom · 2018-11-05T07:39:29Z

NExtends/Primitives/DateTimes/Period.cs

+            }
+        }
+
+        ITimeBlock ITimeBlock.ChangeStartsAt(DateTime startsAt)


dommage d'avoir une implémentation d'interface explicite là où elle peut être normale

au contraire, ici je trouve que Period ne devrait pas avoir ce genre de méthode (car on ne devrait pas essayer de changer une propriété d'un type immutable). Du coup quand tu utilises Period en tant que tel, tu n'auras pas ces méthodes. Y'a alors que sur ITimeBlock, qui est utilisé à la fois par Period et par des types mutables dans le monolithe que ces méthodes sont exposées.

rducom · 2018-11-05T07:42:23Z

NExtends/Primitives/TimeSpans/TimeSpan.extensions.cs

            var absSpan = new TimeSpan(Math.Abs(timeSpan.Ticks));
            var totalHours = Math.Floor(absSpan.TotalHours);
+            var initials = TimeInitials.FromCulture(culture);


on pourrait s'appuyer sur https://github.com/Humanizr/Humanizer pour ce genre de choses

why not, on peut faire ça dans une version ultérieure avec une Issue dédiée.

nfaugout-lucca requested review from GMouron, Poltuu and rducom November 2, 2018 10:59

sonarqubecloud bot reviewed Nov 2, 2018

View reviewed changes

nfaugout-lucca force-pushed the ADD.ITimeBLock branch from f696b4a to cb9268d Compare November 2, 2018 11:11

nfaugout-lucca force-pushed the ADD.ITimeBLock branch from cb9268d to f7c284b Compare November 2, 2018 11:12

sonarqubecloud bot reviewed Nov 2, 2018

View reviewed changes

nfaugout-lucca added 2 commits November 2, 2018 12:19

FIX Sonar

6f4f34a

FIX Sonar : Period is properly derivable

e460dee

rducom requested changes Nov 2, 2018

View reviewed changes

sonarqubecloud bot reviewed Nov 3, 2018

View reviewed changes

FIX Sonar

cb618ee

ADD Tests

rducom reviewed Nov 5, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD : Period & ITimeBlock depuis Lucca #63

ADD : Period & ITimeBlock depuis Lucca #63

nfaugout-lucca commented Nov 2, 2018

sonarqubecloud bot left a comment

codecov bot commented Nov 2, 2018 •

edited

Loading

sonarqubecloud bot left a comment

rducom left a comment

rducom Nov 2, 2018

sonarqubecloud bot left a comment

nfaugout-lucca commented Nov 3, 2018

rducom left a comment

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

rducom Nov 5, 2018

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

rducom Nov 5, 2018

nfaugout-lucca Nov 5, 2018

ADD : Period & ITimeBlock depuis Lucca #63

Are you sure you want to change the base?

ADD : Period & ITimeBlock depuis Lucca #63

Conversation

nfaugout-lucca commented Nov 2, 2018

sonarqubecloud bot left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2018 • edited Loading

Codecov Report

sonarqubecloud bot left a comment

Choose a reason for hiding this comment

rducom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot left a comment

Choose a reason for hiding this comment

nfaugout-lucca commented Nov 3, 2018

rducom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Nov 2, 2018 •

edited

Loading