fix: torna DictionaryExtensions public e remove using redundante#103
Conversation
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. |
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| /// a built-in GetValueOrDefault method. | ||
| /// </summary> | ||
| internal static class DictionaryExtensions | ||
| public static class DictionaryExtensions |
There was a problem hiding this comment.
🔴 Polyfill helper class made public causes ambiguous method errors for consumers using both packages
The shared dictionary helper is changed from internal to public (public static class DictionaryExtensions at src/Shared/DictionaryExtensions.cs:8), but the same source file is compiled into both library assemblies, so consumers referencing both packages on older runtimes get a duplicate-type compilation error.
Impact: Any project targeting .NET Framework or .NET Standard 2.0 that references both Metar.Decoder and Taf.Decoder will fail to compile with an ambiguous call error.
Mechanism: shared source file compiled into two assemblies with public visibility
The file src/Shared/DictionaryExtensions.cs is included in both src/Metar.Decoder/Metar.Decoder.csproj:71 and src/Taf.Decoder/Taf.Decoder.csproj:71 via <Compile Include>. When the class was internal, each assembly had its own private copy invisible to consumers. Now that it's public, both assemblies export System.Collections.Generic.DictionaryExtensions.GetValueOrDefault<TKey, TValue> with the same signature. A downstream project referencing both assemblies on netstandard2.0 or net48 targets will encounter CS0121 (ambiguous call) when any code calls GetValueOrDefault on a dictionary.
The fix is to revert the class back to internal static class DictionaryExtensions.
| public static class DictionaryExtensions | |
| internal static class DictionaryExtensions |
Was this helpful? React with 👍 or 👎 to provide feedback.
| #if NETFRAMEWORK || NETSTANDARD2_0 | ||
| using System.Collections.Generic; | ||
|
|
||
| namespace System.Collections.Generic |
There was a problem hiding this comment.
📝 Info: Redundant using directive removal is correct
The removed using System.Collections.Generic; at the old line 2 was indeed redundant because the file declares its own namespace as System.Collections.Generic (line 2 in the new file). Types within a namespace don't need to import their own namespace, so this cleanup is valid and has no behavioral impact.
Was this helpful? React with 👍 or 👎 to provide feedback.
| /// a built-in GetValueOrDefault method. | ||
| /// </summary> | ||
| internal static class DictionaryExtensions | ||
| public static class DictionaryExtensions |
There was a problem hiding this comment.
🔍 No apparent motivation for the internal-to-public visibility change
There is no InternalsVisibleTo attribute anywhere in the repository, and all usages of GetValueOrDefault are within the two library projects themselves (src/Metar.Decoder/ChunkDecoder/CloudChunkDecoder.cs:87-88 and src/Taf.Decoder/ChunkDecoder/CloudChunkDecoder.cs:75-76). Since the extension method is only called internally and the class was already accessible within each assembly as internal, there appears to be no functional reason to make it public. The commit message says "fix: torna DictionaryExtensions public e remove using redundante" but doesn't explain why public visibility is needed. This should be investigated — if a SonarCloud or Qodana rule triggered this change, the rule should be suppressed rather than exposing an internal polyfill publicly.
Was this helpful? React with 👍 or 👎 to provide feedback.
Qodana Community for .NETIt seems all right 👌 No new problems were found according to the checks applied 💡 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:
New Feature Submissions:
Changes to Core Features:
Summary
Ajustes finais após Devin Review e Qodana no PR #102.
Código:
src/Shared/DictionaryExtensions.cs:publicnovamente para evitar redução de API pública em consumidoresnetstandard2.0/net48using System.Collections.Genericredundante (namespace do arquivo já éSystem.Collections.Generic)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 passandoMotivação
O Devin Review do PR #102 apontou que
internalna classeDictionaryExtensionsseria uma redução de API pública. Manterpublicpreserva compatibilidade para consumidores que utilizam o polyfill emnetstandard2.0/net48. A remoção dousingredundante resolve o warning do Qodana.Link to Devin session: https://app.devin.ai/sessions/94a258020ea4488c8eecc672035d0da1
Requested by: @afonsoft