Document the public members a pragma was hiding, and drop the pragmas that hid nothing (#585) - #943
Merged
Merged
Conversation
…agmas that hid nothing (#585) The library builds with TreatWarningsAsErrors and does not silence CS1591, so the public surface looks documented. It is documented except where a pragma turns the warning off, and there were twenty such regions outside the generated parser. Counting warnings said nothing; counting the suppressions did. Two of the twenty were suppressing nothing at all. The largest -- 588 lines of Entity.Omni.Classes.cs -- carried "TODO: it's only for records' parameters! Remove it once you can document records parameters", and the compiler stopped needing that some versions ago. Entity.Continuous.Calculus is the other. Both come out with no documentation written, because there was nothing missing. Of the rest, most were hiding one thing: the NodeChild, NodeFirstChild and NodeSecondChild accessors that implement IUnaryNode and IBinaryNode. The interface documents them and the members beside them in the same records already use <inheritdoc/>, so 62 of them now do too. That clears nine files: Discrete, Piecewise, Rounding, Trigonometry, ArcTrigonometry, Operators, Exponential, Floors, AbsSignum and Factorial. Note what the pragma comments claimed while they did it -- "only while records' parameters cannot be documented" -- when what they were hiding was an ordinary property. Written rather than inherited, where there was something to say: Complex's operators and conversions, about thirty of them. These share a surprise worth stating: with MathS.Settings.DowncastingEnabled, which is the default, an exact whole value arrives as an Integer and an exact ratio as a Rational, so the runtime type is not the declared one. The float and double conversions read the binary value that was stored rather than the decimal that was written, and decimal does not, which is the kind of thing a caller finds out from a wrong answer. Set's Domain-to-Entity conversion, Piecewise.Cases, and the two members of Matrices.Direction. Eleven of the twenty regions are gone. The nine that remain are the operator and conversion blocks on Real, Rational, Integer, Number, Matrix and the implicit operators -- about 150 members of the same kind as Complex's, and the same treatment applies to them. The generated parser keeps its pragma: it is regenerated from the grammar, so anything written into it is written to be lost. Measured: suite 7143 passed / 0 failed, and the build itself is the check -- CS1591 is an error here, so a pragma cannot come out unless every member under it is documented. #585 #746 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Entity's implicit conversions from the numeric types, Number's arithmetic operators, and the tensor operations a matrix of expressions is built on. Three more suppression regions gone, leaving four. Two things were worth saying rather than restating the signature. The conversions to Entity produce the narrowest literal that holds the value exactly, and the float and double ones read the binary value that was stored rather than the decimal that was written, while decimal reads what was written. A caller who does not know that finds it out from an answer. EntityTensorWrapperOperations normalises after every operation with InnerSimplified, and the reason is worth recording: a determinant over a 4x4 of symbols is a sum of products of sums, and an entry left unnormalised accumulates the whole history of the operations that built it. Its equality is structural rather than mathematical, so x + x and 2 * x are not equal there, and IsZero recognises the literal zero rather than an expression that happens to vanish. Measured: build succeeds, and the build is the check -- CS1591 is an error here. #585 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ng NaN does (#585) Three more suppression regions, leaving one -- Number's, which is left alone deliberately because the BigInteger fix is editing inside it and two branches on one block conflict for the second one. Most of these are the arithmetic and the conversions, and they say the two things a caller cannot see from the signature: comparison and arithmetic here are on the value and answer a number or a bool, where the same operators on Entity build a node to be evaluated later; and downcasting, which is on by default, means an exactly whole value handed to Real or Rational arrives as an Integer, so the runtime type is narrower than the declared one. Three answers that surprise, now written where they are: Integer / Integer is not integer division. 1 / 2 is a half, which is why it returns Real: a Rational wherever the division is not exact, and NaN where the divisor is zero. IntegerDiv is the truncating one. Rational / Rational returns Real for the same reason -- every quotient of two ratios is a ratio except division by zero, and NaN is not a ratio. A ratio arrives in lowest terms with a positive denominator: 2/4 is 1/2 and 1/(-2) is (-1)/2. Measured rather than assumed. And one that surprised me while writing it down. Real's comparison operators order NaN instead of refusing to: NaN > 1 and NaN >= 1 are both true, and 1 < NaN is true as well. It is a total order with NaN at the top, and it is not what double does, where every comparison against NaN is false. The first draft of this documentation said the opposite, from habit, and measuring at +1, -1, 0 and NaN is what caught it. RealComparisonTest pins it, because documentation now asserts it. Whether that ordering is the right answer is a separate question from documenting it, and this changes no behaviour. Measured: suite 7123 passed / 0 failed, 6 of them new. The build is the other check -- CS1591 is an error here, so a pragma cannot come out unless everything under it is documented. #585 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The library builds with
TreatWarningsAsErrorsand does not silence CS1591, so the public surface looks documented. It is documented — except where a pragma turns the warning off, and there were twenty such regions outside the generated parser.Counting warnings said nothing here. Counting the suppressions did.
Two of the twenty were suppressing nothing at all
The largest — 588 lines of
Entity.Omni.Classes.cs— carried:The compiler stopped needing that some versions ago.
Entity.Continuous.Calculus.Classes.csis the other. Both come out with no documentation written, because nothing was missing. A stale TODO had been hiding an empty room.Most of the rest were hiding one thing
NodeChild,NodeFirstChildandNodeSecondChild— theIUnaryNode/IBinaryNodeimplementations. The interface documents them, and the members beside them in the same records already use<inheritdoc/>, so 62 of them now do too.That clears nine more files: Discrete, Piecewise, Rounding, Trigonometry, ArcTrigonometry, Operators, Exponential, Floors, AbsSignum, Factorial.
Worth noting what those pragmas claimed while they did it — "only while records' parameters cannot be documented" — when what they were actually hiding was an ordinary property.
Written, where there was something to say
Complex's operators and conversions, about thirty. These share a surprise worth stating rather than leaving to be discovered:MathS.Settings.DowncastingEnabled(the default) an exact whole value arrives as anIntegerand an exact ratio as aRational, so the runtime type is not the declared one;floatanddoubleconversions read the binary value that was stored, not the decimal that was written — anddecimaldoes not. That is the kind of thing a caller otherwise finds out from a wrong answer.Plus
Set'sDomain→Entityconversion,Piecewise.Cases, and the two members ofMatrices.Direction.Where it stands
Eleven of the twenty regions are gone. The nine that remain are the operator and conversion blocks on
Real,Rational,Integer,Number,Matrixand the implicit operators — about 150 members of the same kind asComplex's, and the same treatment applies.The generated parser keeps its pragma: it is regenerated from the grammar, so anything written into it is written to be lost.
Measured
Suite 7117 passed / 0 failed — and the build is itself the check. CS1591 is an error here, so a pragma cannot come out unless every member beneath it is documented. Nothing in this PR changes behaviour, and no public member is added or removed, so
PublicApi.txtis untouched.Toward #585 and #746 v1.0's "documentation of every public surface".
🤖 Generated with Claude Code