Open
Conversation
…of(), TolerantStringEnumConverter - Replace == null/!= null with is null/is not null throughout - Use uppercase String.IsNullOrWhiteSpace/String.Empty convention - Use nameof() for argument validation parameter names - Add TolerantStringEnumConverter for resilient JSON deserialization
…oval - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Add missing GoogleAddressType values (AdminLevel4-7, PlusCode, Landmark, etc.) - Add Route and Locality to GoogleComponentFilterType - Add OverDailyLimit and UnknownError to GoogleStatus - Remove obsolete sensor=false parameter from API URLs
- Add AzureMapsGeocoder with search/reverse geocoding, culture, bounds bias - Add AzureMapsAddress with EntityType, ConfidenceLevel, neighborhood support - Fix Bing EntityType parsing: use TryParse with Unknown fallback (crash fix) - Add Unknown value to EntityType enum at position 0 - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation
…style - Migrate from legacy AppId/AppCode to API key authentication - Update to HERE Geocoding v1 (v7) REST endpoints - Replace custom Json.cs with Newtonsoft.Json deserialization - Remove legacy two-parameter constructor (throws NotSupportedException) - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Document HereLocationType legacy v6.2 values in XML remarks
…r, code style - Disable OpenStreetMap mode (throws NotSupportedException) - Add TolerantStringEnumConverter for resilient Quality/SideOfStreet parsing - Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Fix DataFormat.json value for correct API requests
- Replace == null/!= null with is null/is not null - Use uppercase String.* convention and nameof() for validation - Mark as deprecated but keep functional for contributors with credentials
- Rename all test methods to Method_Scenario_ExpectedResult format - Add Arrange/Act/Assert comments to multi-phase tests - Add Azure Maps test coverage (AzureMapsAsyncTest) - Add ProviderCompatibilityTest for cross-provider validation - Fix Yahoo tests: remove hardcoded Skip, use credential gating via SettingsFixture - Add yahooConsumerKey/yahooConsumerSecret to settings template - Use uppercase String.* convention in test infrastructure
- Update README with Azure Maps provider, HERE API key migration notes - Mark Yahoo as deprecated in documentation - Update Example.Web sample with Azure Maps and modern patterns - Add docs/plan.md to .gitignore to prevent leaking internal planning docs - Update VS Code settings for workspace
Add <Nullable>enable</Nullable> to Directory.Build.props so all projects enforce nullable analysis. Include a NullableAttributes.cs polyfill for NotNullWhenAttribute on netstandard2.0 where System.Diagnostics.CodeAnalysis is not available.
Replace Newtonsoft.Json with System.Text.Json across the Core library: - Replace JsonConvert with JsonSerializer in Extensions.cs - Extract TolerantStringEnumConverter as a JsonConverterFactory for STJ - Create frozen, shared JsonSerializerOptions with MakeReadOnly() - Add System.Text.Json NuGet dependency - Annotate all public types with nullable reference type annotations - Add [NotNullWhen(false)] to IsNullOrEmpty<T> extension - Add protected parameterless constructors to Address and ParsedAddress for STJ deserialization support
Replace Newtonsoft.Json attributes with System.Text.Json equivalents and annotate all types with nullable reference type annotations.
… exception handling - Replace DataContractJsonSerializer with System.Text.Json - Remove dead Bing Maps Routing API types (Route, RouteLeg, RoutePath, Shape, Warning, etc.) that were never used by the geocoding library - Type ResourceSet.Resources directly as Location[] instead of Resource[] - Decouple AzureMapsAddress from BingAddress with its own backing fields - Apply 'when' exception filters to prevent double-wrapping exceptions that are already typed as BingGeocodingException - Add null guards in ParseResponse for missing Point/Address data - Annotate all types with nullable reference type annotations - Remove Newtonsoft.Json dependency from csproj
…nnotations - Replace Newtonsoft.Json with System.Text.Json for API response parsing - Apply 'when' exception filters to prevent double-wrapping HereGeocodingException - Remove Newtonsoft.Json dependency from csproj - Annotate all types with nullable reference type annotations
… annotations - Replace Newtonsoft.Json with System.Text.Json across all MapQuest types - Add protected [JsonConstructor] parameterless constructor to MapQuestLocation to resolve STJ parameter binding conflict between FormattedAddress property (mapped to 'location' in JSON) and the constructor parameter - Fix FormattedAddress setter to silently ignore null/blank values during deserialization instead of throwing - Improve ToString() to handle empty base FormattedAddress - Annotate all types with nullable reference type annotations
Yahoo BOSS Geo Services has been discontinued. Mark all public types with [Obsolete] attributes directing users to alternative providers. Add NoWarn for CS0618 in csproj to suppress internal obsolete usage. Annotate types with nullable reference type annotations.
- Add Arrange/Act/Assert section comments to all test methods - Fix nullable warnings with null! for test field initialization - Make theory parameters nullable where InlineData passes null - Add ! operator for reflection-based code in ProviderCompatibilityTest - Modernize SettingsFixture to read API keys from environment variables with GEOCODING_ prefix via AddEnvironmentVariables - Add xunit.v3.assert.source package for xUnit v3 compatibility - Restore regression test comment on MapQuest neighborhood test
…andling - Align Google provider to use 'when' exception filter pattern consistent with Bing, Azure Maps, and HERE providers - Replace null-forgiving operator with defensive Array.Empty fallback in MapQuest Execute method
Google integration failures were caused by the configured Google Cloud project having the Geocoding API disabled, which surfaced as REQUEST_DENIED and burned through the Google test matrix. Preserve Google's provider error message in GoogleGeocodingException, add a one-time cached Google availability guard that skips Google integration tests after the first denied request, and add non-network tests that lock down the parser-to-exception behavior.
Drop the reflection-based parser test and keep the direct exception coverage. The Google RCA is already proven by the live targeted integration check, so the extra private-method test added complexity without enough value.
With the enabled Google key, the remaining Google failure was not a provider error but a ZERO_RESULTS response for the live Rothwell + NN14 component-filter query. Replace that brittle integration case with a deterministic ServiceUrl test that still verifies postal_code component filter construction without depending on drifting Google geocoder data.
The test fixture was still reading legacy flat keys while the sample app binds a Providers section. Teach SettingsFixture to prefer Providers:*:ApiKey values and fall back to the legacy flat keys so local overrides keep working while test and sample configuration stay consistent.
The audit found two test-integrity regressions on this branch: shared White House assertions had been weakened, and the Google postal-code filter coverage was reduced to a URL-construction check. Restore the stricter shared assertions and reinstate live postal-code filter coverage with a stable Google case that returns OK today.
The branch docs and README say Yahoo remains deprecated and unverified, but the test suite had been re-enabled by deleting the old skip wrappers. Restore an explicit centralized skip in YahooGeocoderTest so the branch behavior matches the documented provider status and upstream OAuth issue history.
The full test audit found remaining expectation drift and a few integrity regressions in the branch. Align the async base White House query with the sync base, restore strict shared assertions, update Google live expectations to match current provider responses, keep Yahoo explicitly skipped while deprecated, and scope obsolete warning suppression to the Yahoo compatibility test only.
The bounds-bias test still needed to tolerate ZIP-code drift, but it should not lose the country-level assertion. Keep the stable locality substring check and explicitly require the U.S. country signal in both the formatted address and the parsed address components.
Root cause: the shared test bases eagerly created live geocoder instances in their constructors, which pushed cheap constructor checks into separate files and made the test layout drift away from provider-specific suites. Switching the base tests to lazy generic geocoder access keeps those checks colocated without extra wrapper classes or reflection hacks.
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 90 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Root cause: the Yahoo suite had been hard-skipped even when a user supplied credentials, which prevented intentional compatibility runs. Remove the unconditional Skip attributes so the existing settings gate controls whether the Yahoo tests execute.
Root cause: the HttpClient transport cleanup left several provider failure paths under-tested and changed exception diagnostics/contracts in ways that made regressions easier to miss.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 92 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Root cause: sibling providers still used the pre-hardening HTTP failure path, so status handling and bounded diagnostics were inconsistent after the transport cleanup.
Root cause: the new transport hardening tests still had analyzer noise and one locale-sensitive Google channel edge case without regression coverage.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 92 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Root cause: Location.GetHashCode accidentally combined latitude with itself, so longitude never influenced the hash for this public value-like core type.
Root cause: the latest push triggered one more static-analysis pass that flagged a disposable test-helper pattern, two Yahoo immutability nits, one unreachable Yahoo throw, and avoidable empty-array allocations in MapQuest.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 89 out of 92 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
src/Geocoding.MapQuest/MapQuestGeocoder.cs:285
- This returns
new ResultItem[0], which allocates a new array each call. PreferArray.Empty<ResultItem>()to avoid repeated allocations (especially since the method already usesArray.Empty<...>()in other branches).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Root cause: the provider-modernization branch still had a small set of follow-up inconsistencies around nullable annotations, Bing response handling, and the shared HttpClient test helper after the larger transport and serializer refactors.
Root cause: the monolithic Extensions type mixed collection, enumerable, and JSON behavior in one file, which made the API surface harder to navigate and blocked a cleaner namespace layout for the shared helpers.
Root cause: the first split kept a compatibility shim and legacy JSON method casing, which preserved old entry points but did not match the requested .Extensions namespace layout or the desired .NET-style naming for JSON helpers.
Root cause: the previous revision kept the split helper classes but still invoked them like static utilities, which did not match the requested extension-method style or the naming implied by the new Geocoding.Extensions surface.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 92 out of 95 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
78
to
87
| public int Compare(QueryParameter x, QueryParameter y) | ||
| { | ||
| if (x.Name == y.Name) | ||
| { | ||
| return string.Compare(x.Value, y.Value); | ||
| return String.Compare(x.Value, y.Value); | ||
| } | ||
| else | ||
| { | ||
| return string.Compare(x.Name, y.Name); | ||
| return String.Compare(x.Name, y.Name); | ||
| } |
There was a problem hiding this comment.
QueryParameterComparer.Compare uses String.Compare without specifying a StringComparison, which makes the OAuth parameter sorting culture-sensitive. OAuth 1.0 signature normalization should use an ordinal comparison to ensure signatures are stable across cultures/locales (otherwise signatures can differ under e.g. tr-TR). Consider using StringComparison.Ordinal (or StringComparer.Ordinal) for both name and value comparisons.
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
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.
Summary
Root Cause
The branch started from test-layout and configuration cleanup, but the actual review and implementation scope expanded to provider modernization, parser hardening, and compatibility cleanup across multiple packages.
What Changed and Why
Breaking Changes
Tech Debt Assessment
Test Plan