Skip to content

Feature/change project naming #1

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

Merged

Conversation

MartinCarpentier
Copy link
Contributor

No description provided.

…he SLN file to point to the new route after folder renaming
Updated the code to support system.text.json, and also updated the tests
to run using system.text.json.

Issues that needs fixing:
Could not make the jsonconstructor work properly, so i updated the
properties to be INIT.

This does not solve that the classes can be instantiated invalidly, so
more work might have to go into solving the jsonconstructors.
@MartinCarpentier MartinCarpentier force-pushed the feature/change-project-naming branch from c0050f4 to 1b18a6a Compare December 16, 2021 20:34
@MartinCarpentier MartinCarpentier force-pushed the feature/change-project-naming branch from 1b18a6a to 4abf3ac Compare December 16, 2021 20:39
@MartinCarpentier
Copy link
Contributor Author

MartinCarpentier commented Dec 16, 2021

The code can build, tests work but some things needs to be adressed as explained below.

Changes

  • Changed the code to use system.text.json.
  • Added github actions to build the project using .net core 3.1, .net 5 and .net 6
  • Moved the test geojson files into embedded resources to make it easier to test in github actions

Issues to look into

  • Could not get the jsonconstructors to work properly, so opened the properties, but made them init only.
    • This is not the same as the solution that already exists, since users can create classes which does not force the use as the same properties/ fields as the constructor did.

Copy link
Member

@matt-lethargic matt-lethargic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An amazing PR, didn't think you were going to do it all in one go like that. Thank you. Have a look at my comments and once you've replied/resolved I'll get this in 👍

I sat down to have a look at this last night and I came to the same problem as you did with the constructors. This is something that will have to be lived with for now and addressed as and when.

@matt-lethargic matt-lethargic self-assigned this Dec 17, 2021
@matt-lethargic matt-lethargic merged commit 5a9ecd0 into GeoJSON-Net:main Dec 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants