Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several issues #45758

Merged
merged 4 commits into from
Sep 16, 2024
Merged

Fix several issues #45758

merged 4 commits into from
Sep 16, 2024

Conversation

jecmenicanikola
Copy link
Contributor

This PR contains:

  • Change some namespaces
  • Update docstring
  • Hide geojson classes

@azure-sdk
Copy link
Collaborator

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Maps.Search

@jecmenicanikola
Copy link
Contributor Author

/check-enforcer evaluate

@dubiety
Copy link
Member

dubiety commented Sep 2, 2024

@jecmenicanikola , we should make all the GeoJsonxxx classes as internal class. Please refer to how Routing package achieves this. We should use Azure.Core.GeoJson objects. Routing API doesn't have GeoJson classes exposed. Please see https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/maps/Azure.Maps.Routing/api/Azure.Maps.Routing.netstandard2.0.cs.

@jecmenicanikola
Copy link
Contributor Author

jecmenicanikola commented Sep 3, 2024

            #region Snippet:GetPolygon
            GetPolygonOptions options = new GetPolygonOptions()
            {
                Coordinates = new GeoPosition(-122.204141, 47.61256),
                ResultType = BoundaryResultTypeEnum.Locality,
                Resolution = ResolutionEnum.Small,
            };
            Response<Boundary> result = client.GetPolygon(options);
            var count = ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates.Count;
            for (var i = 0; i < count; i++)
            {
                var coorCount = ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates[i].Count;
                for (var j = 0; j < coorCount; j++)
                {
                    Console.WriteLine(string.Join(",", ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates[i][j]));
                }
            }

this in test package, I need to use GeoJsonGeometryCollection and GeoJsonPolygon to cast, so these classes cannot be internal. I don't have corresponding classes in Azure.Core.GeoJson.

@jecmenicanikola
Copy link
Contributor Author

@dubiety what about my commnet? Are we good to merge this?

@dubiety
Copy link
Member

dubiety commented Sep 9, 2024

@dubiety what about my commnet? Are we good to merge this?

Using it only for casting doesn't seem right to me. Let me check and I'll get back to you soon.

@dubiety
Copy link
Member

dubiety commented Sep 10, 2024

#region Snippet:GetPolygon GetPolygonOptions options = new GetPolygonOptions() { Coordinates = new GeoPosition(-122.204141, 47.61256), ResultType = BoundaryResultTypeEnum.Locality, Resolution = ResolutionEnum.Small, }; Response<Boundary> result = client.GetPolygon(options); var count = ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates.Count; for (var i = 0; i < count; i++) { var coorCount = ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates[i].Count; for (var j = 0; j < coorCount; j++) { Console.WriteLine(string.Join(",", ((GeoJsonPolygon)((GeoJsonGeometryCollection)result.Value.Geometry).Geometries[0]).Coordinates[i][j])); } } this in test package, I need to use GeoJsonGeometryCollection and GeoJsonPolygon to cast, so these classes cannot be internal. I don't have corresponding classes in Azure.Core.GeoJson.

For GeoJsonGeometryCollection Can you consider using GeoCollection? I think we should be able to map the object below:

Azure Maps SDK Azure.Core.GeoJson
GeoJsonGeometryCollection GeoCollection
GeoJsonGeometry GeoObject

Azure.Core.GeoJson source code

@jecmenicanikola
Copy link
Contributor Author

@dubiety I couldn't map these objects

Azure Maps SDK Azure.Core.GeoJson
GeoJsonGeometryCollection GeoCollection
GeoJsonGeometry GeoObject

@dubiety
Copy link
Member

dubiety commented Sep 16, 2024

@dubiety I couldn't map these objects

Azure Maps SDK Azure.Core.GeoJson
GeoJsonGeometryCollection GeoCollection
GeoJsonGeometry GeoObject

It's fine. I'll fix it. I approved your PR and will merge it.

@dubiety dubiety merged commit 78ed4d1 into Azure:main Sep 16, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants