-
Notifications
You must be signed in to change notification settings - Fork 385
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate operators from unit relations defined in JSON #1329
Changes from 3 commits
d93e058
ce1a6ff
cfefd26
c7918a3
4de9789
7046318
59e8f7f
049eaf9
7a52a91
5c9abed
2b9d456
f4231c1
5802fb2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,33 +31,37 @@ public static void ParseAndApplyRelations(string rootDir, Quantity[] quantities) | |
var quantityDictionary = quantities.ToDictionary(q => q.Name, q => q); | ||
|
||
// Add double and 1 as pseudo-quantities to validate relations that use them. | ||
var pseudoQuantity = new Quantity { Name = null!, Units = new[] { new Unit { SingularName = null! } } }; | ||
var pseudoQuantity = new Quantity { Name = null!, Units = [new Unit { SingularName = null! }] }; | ||
quantityDictionary["double"] = pseudoQuantity with { Name = "double" }; | ||
quantityDictionary["1"] = pseudoQuantity with { Name = "1" }; | ||
|
||
var relations = ParseRelations(rootDir, quantityDictionary); | ||
var timespanRelations = new List<QuantityRelation>(); | ||
|
||
foreach (var relation in relations) | ||
{ | ||
if (relation.LeftQuantity.Name == "Duration") | ||
// Because multiplication is commutative, we can infer the other operand order. | ||
relations.AddRange(relations | ||
.Where(r => r.Operator is "*" or "inverse" && r.LeftQuantity != r.RightQuantity) | ||
.Select(r => r with | ||
{ | ||
timespanRelations.Add(relation with | ||
{ | ||
LeftQuantity = new Quantity { Name = "TimeSpan" }, | ||
}); | ||
} | ||
|
||
if (relation.RightQuantity.Name == "Duration") | ||
{ | ||
timespanRelations.Add(relation with | ||
{ | ||
RightQuantity = new Quantity { Name = "TimeSpan" }, | ||
}); | ||
} | ||
} | ||
|
||
relations.AddRange(timespanRelations); | ||
LeftQuantity = r.RightQuantity, | ||
LeftUnit = r.RightUnit, | ||
RightQuantity = r.LeftQuantity, | ||
RightUnit = r.LeftUnit, | ||
}) | ||
.ToList()); | ||
|
||
// We can infer TimeSpan relations from Duration relations. | ||
var timeSpanQuantity = pseudoQuantity with { Name = "TimeSpan" }; | ||
relations.AddRange(relations | ||
.Where(r => r.LeftQuantity.Name is "Duration") | ||
.Select(r => r with { LeftQuantity = timeSpanQuantity }) | ||
.ToList()); | ||
relations.AddRange(relations | ||
.Where(r => r.RightQuantity.Name is "Duration") | ||
.Select(r => r with { RightQuantity = timeSpanQuantity }) | ||
.ToList()); | ||
|
||
// Sort all relations to keep generated operators in a consistent order. | ||
relations.Sort(); | ||
|
||
foreach (var quantity in quantities) | ||
{ | ||
|
@@ -67,24 +71,13 @@ public static void ParseAndApplyRelations(string rootDir, Quantity[] quantities) | |
{ | ||
if (relation.LeftQuantity == quantity) | ||
{ | ||
// The left operand of a relation is responsible for generating the operator. | ||
quantityRelations.Add(relation); | ||
if (relation is { Operator: "*", RightQuantity.Name: "TimeSpan" or "double" }) | ||
{ | ||
quantityRelations.Add(relation.Swapped()); | ||
} | ||
} | ||
|
||
if (relation.RightQuantity == quantity) | ||
else if (relation.RightQuantity == quantity && relation.LeftQuantity.Name is "double" or "TimeSpan") | ||
{ | ||
if (relation.Operator == "inverse" || relation.Operator == "*" && relation.LeftQuantity != relation.RightQuantity) | ||
{ | ||
quantityRelations.Add(relation.Swapped()); | ||
} | ||
|
||
if (relation is { Operator: "/", LeftQuantity.Name: "double" }) | ||
{ | ||
quantityRelations.Add(relation); | ||
} | ||
// Because we cannot add generated operators to double or TimeSpan, we make the right operand responsible in this case. | ||
quantityRelations.Add(relation); | ||
} | ||
} | ||
|
||
|
@@ -99,7 +92,7 @@ private static List<QuantityRelation> ParseRelations(string rootDir, IReadOnlyDi | |
try | ||
{ | ||
var text = File.ReadAllText(relationsFileName); | ||
var relationStrings = JsonConvert.DeserializeObject<List<string>>(text) ?? new List<string>(); | ||
var relationStrings = JsonConvert.DeserializeObject<List<string>>(text) ?? []; | ||
relationStrings.Sort(); | ||
|
||
var parsedRelations = relationStrings.Select(relationString => ParseRelation(relationString, quantities)).ToList(); | ||
|
@@ -119,7 +112,7 @@ private static QuantityRelation ParseRelation(string relationString, IReadOnlyDi | |
{ | ||
var segments = relationString.Split(' '); | ||
|
||
if (segments.Length != 5 || segments[1] != "=") | ||
if (segments is not [_, "=", _, "*" or "/", _]) | ||
{ | ||
throw new Exception($"Invalid relation string: {relationString}"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Exception can give an example of a valid format. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could, but isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolutely, but I still think it improves the developer experience a bit. |
||
} | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we throw on duplicates here, with a helpful explanation?
It probably gives compile errors in the generated code anyway, but it would be helpful if codegen failed early on invalid input to make it easier for contributors to find out what they did wrong.
If we eventually do #1354 , then duplicates could also occur implicitly by one explicit definition conflicting with the implicit definition for division operators.
Another option is to remove duplicates with a HashSet or similar, then fix the document when writing it back. But it may be complicated to know what to remove, in particular with the magic division operators, so it is probably better to just throw and have the author fix their mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I added a check after all relations are inferred.
Duplicate definitions in
UnitRelations.json
are automatically removed with aSortedSet
.