fix: ajustes pós-merge PR #99 (Devin Review, CodeQL, duplicação)#100
Conversation
- Corrige Remaining não setado no success path de EvolutionChunkDecoder.Parse - Converte int*100 para double para evitar overflow CodeQL - Usa TryGetValue em ApplyDecodedData (METAR/TAF) - Valida result não nulo em ApplyDecodedChunk (TAF) - Reduz duplicação em CloudChunkDecoder TAF com Dictionary mapping Co-Authored-By: Afonso Dutra Nogueira Filho <[email protected]>
🤖 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. |
|
Qodana Community for .NET5 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]
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 CAVOK flag is unconditionally reset to false inside evolution entity parsing loop
At EvolutionChunkDecoder.cs:115, _withCavok is set to false on every iteration of the decoder chain loop, even though ResolveEntityName at line 166 may set it to true when a CAVOK visibility chunk is encountered. This means if the VisibilityChunkDecoder runs and sets _withCavok = true via ResolveEntityName, the next decoder iteration immediately resets it to false. This is a pre-existing issue (not introduced by this PR) but the PR touches the surrounding method, so it's worth noting. The impact may be limited since _withCavok is primarily used in AddEvolution to set newEvolution.Cavok, which would have already been called by that point.
(Refers to line 115)
Was this helpful? React with 👍 or 👎 to provide feedback.
| } | ||
|
|
||
| ParseEntitiesChunk(evolution, remaining, decodedTaf); | ||
| Remaining = ParseEntitiesChunk(evolution, remaining, decodedTaf); |
There was a problem hiding this comment.
📝 Info: Pre-existing bug fixed: Remaining property was never set on successful evolution parse
In the old code, EvolutionChunkDecoder.Parse only set Remaining when the regex match failed (found.Count <= 1). When the match succeeded, ParseEntitiesChunk was called but Remaining was never updated. The caller in TafDecoder.cs:166-167 reads evolutionDecoder.Remaining after every call in a while loop. On the first successful parse, Remaining would be null (its default), causing the while loop's !string.IsNullOrEmpty(remainingTaf) check to exit early — potentially dropping all subsequent evolution blocks (BECMG, TEMPO, FM) from the decoded TAF. The new code at line 95 correctly sets Remaining to the return value of ParseEntitiesChunk, which is the text left after entity decoders have consumed their portions. This is a meaningful bug fix.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (decodedData.TryGetValue(ExceptionKey, out var exception) && exception is MetarChunkDecoderException metarChunkDecoderException) | ||
| { | ||
| decodedMetar.AddDecodingException((MetarChunkDecoderException)decodedData[ExceptionKey]); | ||
| decodedMetar.AddDecodingException(metarChunkDecoderException); | ||
| } |
There was a problem hiding this comment.
📝 Info: Subtle behavioral change: hard cast replaced with pattern match silently skips non-matching types
The old code used (MetarChunkDecoderException)decodedData[ExceptionKey] which would throw InvalidCastException if the value was not the expected type. The new code uses exception is MetarChunkDecoderException metarChunkDecoderException which silently skips if the type doesn't match. In practice this is safe because ExceptionKey is only ever populated with a MetarChunkDecoderException at MetarDecoder.cs:202, but it is a semantic change from "crash on unexpected type" to "silently ignore unexpected type." This is arguably more robust but worth being aware of.
Was this helpful? React with 👍 or 👎 to provide feedback.
| private static readonly Dictionary<string, CloudLayer.CloudAmount> AmountMap = new Dictionary<string, CloudLayer.CloudAmount> | ||
| { | ||
| { "FEW", CloudLayer.CloudAmount.FEW }, | ||
| { "SCT", CloudLayer.CloudAmount.SCT }, | ||
| { "BKN", CloudLayer.CloudAmount.BKN }, | ||
| { "OVC", CloudLayer.CloudAmount.OVC }, | ||
| { "VV", CloudLayer.CloudAmount.VV }, | ||
| }; | ||
|
|
||
| private static readonly Dictionary<string, CloudLayer.CloudType> TypeMap = new Dictionary<string, CloudLayer.CloudType> | ||
| { | ||
| { "CB", CloudLayer.CloudType.CB }, | ||
| { "TCU", CloudLayer.CloudType.TCU }, | ||
| { "///", CloudLayer.CloudType.CannotMeasure }, | ||
| }; |
There was a problem hiding this comment.
🔍 Inconsistent refactoring style between Metar and Taf CloudChunkDecoders
The Taf CloudChunkDecoder was refactored to use static Dictionary lookups (AmountMap, TypeMap) instead of switch expressions, while the Metar CloudChunkDecoder at src/Metar.Decoder/ChunkDecoder/CloudChunkDecoder.cs:84-105 retains the original switch expression approach (ParseCloudAmount, ParseCloudType). Both decoders serve the same purpose for their respective domains and the AGENTS.md rule S2 says "Adicionar novo ChunkDecoder → seguir padrão existente." Having two different patterns for the same logic across Metar and Taf may cause confusion for future contributors.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
All Submissions:
New Feature Submissions:
Changes to Core Features:
Summary
Ajustes no PR #99 para resolver os apontamentos do Devin Review, CodeQL e SonarCloud.
Correções:
EvolutionChunkDecoder.Parseagora atribuiRemainingno caminho de sucesso, permitindo queTafDecoder.DecodeEvolutionsprocesse múltiplas evoluções corretamenteParseEntitiesChunkvoltou a retornarstringe o retorno é utilizadoCloudChunkDecoderTAF passou a usarDictionarypara mapeamento deCloudAmount/CloudType, reduzindo duplicação com o METARlayerHeight.Value * 100.0(double) para evitar alerta de overflow do CodeQLApplyDecodedData(METAR/TAF) usaTryGetValuepara evitar acesso duplo ao dicionárioresultnão nulo emApplyDecodedChunk(TAF)Validação
dotnet build MetarDecoder.sln --configuration ReleaseOKdotnet test tests/Metar.Decoder.Tests/(net8.0/net10.0) — 242/242 passandodotnet test tests/Taf.Decoder.Tests/(net8.0/net10.0) — 192/192 passandoLink to Devin session: https://app.devin.ai/sessions/94a258020ea4488c8eecc672035d0da1
Requested by: @afonsoft