fix: address chunk decoder review notes#84
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| var measuredValue = Value.ToInt(value); | ||
| return measuredValue.HasValue ? new Value(measuredValue.Value, unit) : null; |
There was a problem hiding this comment.
📝 Info: Silent null return in ToValue changes error-reporting behavior for hypothetical edge cases
The old ToValue would throw InvalidOperationException (accessing .Value on a null Nullable<int>) if Value.ToInt returned null. The new code at src/Metar.Decoder/ChunkDecoder/RunwayVisualRangeChunkDecoder.cs:111 silently returns null instead. This means a RunwayVisualRange object could end up with a null VisualRange or null entries in VisualRangeInterval, which downstream consumers may not expect.
In practice this path is unreachable because the regex group ([0-9]{4}) guarantees a 4-digit numeric string, and Value.ToInt at src/Metar.Decoder/Entity/Value.cs:150-158 will always successfully parse such input. However, if the regex were ever relaxed to accept non-numeric characters in these groups, the silent null would mask a parsing failure rather than surfacing it as an exception.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Concordo; ajustei ToValue para lançar em falha inesperada de parse, preservando o comportamento fail-fast em vez de retornar null silenciosamente.
| { | ||
| var qfuAsInt = Value.ToInt(runway); | ||
| if (qfuAsInt > 36 || qfuAsInt < 1) | ||
| if (!qfuAsInt.HasValue || qfuAsInt.Value > 36 || qfuAsInt.Value < 1) |
There was a problem hiding this comment.
📝 Info: Runway number 00 is now rejected by the null check, but was already rejected before
The new !qfuAsInt.HasValue guard at line 81 adds a null check, but for runway designators matched by the regex ([0-9]{2}[LCR]?), Value.ToInt will always return a non-null integer. The existing < 1 check already rejects runway 00 (which is the only invalid low value the regex can produce). The old code relied on C#'s implicit Nullable<int> comparison operators (where null > 36 and null < 1 both return false), meaning a null ToInt result would have passed validation silently. The new explicit null check correctly treats a null parse result as invalid. This is a genuine correctness improvement for the theoretical null case, even though the regex makes it unreachable.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Intenção é falhar fechado em entrada numérica inesperada; o caso nulo é inalcançável com a regex atual, mas o guard torna a invariável explícita.
Qodana Community for .NET4 new problems were found
💡 Qodana analysis was run in the pull request mode: only the changed files were checked View the detailed Qodana reportTo be able to view the detailed Qodana report, you can either:
To get - name: 'Qodana Scan'
uses: JetBrains/[email protected]
with:
upload-result: trueContact Qodana teamContact us at [email protected]
|
1 participant
All Submissions:
Changes to Core Features:
RunwayVisualRangeChunkDecoderhelpers so nullable runway values are handled explicitly instead of via.Valueon nullable results.WindShearChunkDecoderregex string to remove the redundant verbatim prefix.dotnet build MetarDecoder.sln --configuration Releasedotnet test tests/Metar.Decoder.Tests/Metar.Decoder.Tests.csproj --configuration Release --framework net8.0 --collect:"XPlat Code Coverage"dotnet test tests/Metar.Decoder.Tests/Metar.Decoder.Tests.csproj --configuration Release --framework net10.0 --collect:"XPlat Code Coverage"dotnet test tests/Taf.Decoder.Tests/Taf.Decoder.Tests.csproj --configuration Release --framework net8.0 --collect:"XPlat Code Coverage"dotnet test tests/Taf.Decoder.Tests/Taf.Decoder.Tests.csproj --configuration Release --framework net10.0 --collect:"XPlat Code Coverage"Link to Devin session: https://app.devin.ai/sessions/a4e91bc6cd1447788a7bec8c00406d09