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

Added possibility to add rootkey for objects #4

Merged
merged 4 commits into from
Jul 24, 2023

Conversation

nicolaspierre1990
Copy link
Contributor

Hello,

I have added the possibilty to add rootkey to the object.

Because netcore3.1 is out of LTS support I have updated the test project to NET6 and the main project to netstandard2.1

Updated version to 1.1.0 for breaking changes

Added possibility to add rootkey for objects
update dependencies
@lawrence-laz
Copy link
Owner

lawrence-laz commented Jul 24, 2023

Hi,

Thanks for the PR! I'll make sure to look through it a bit later.

Just a couple of things right off the bat:

  1. I'd like to follow the semantic versioning where a breaking change triggers a major bump, so this would be 2.0.0. I should probably update the readme regarding the use of semantic versioning 😀
  2. If possible, I'd like to avoid making breaking changes by:
    • Sticking the the library to netstandard2.0 for compatibility with .net framework (some unfortunate people still have to use it, including me). NET6 bump is fine.
    • Creating an overload of the method (and constructor) instead of modifying the existing one, not to break ABI and to avoid using default parameters (they can be confusing especially in package code)

Let me know if that's possible with what you are trying to do

@lawrence-laz
Copy link
Owner

All looks good, except for the couple things mentioned above and the pipeline needs updating

We can address those here, or I can fix them up after merging this PR. Whichever you prefer?

@nicolaspierre1990
Copy link
Contributor Author

I'll take a look at them later today

@nicolaspierre1990
Copy link
Contributor Author

@lawrence-laz I have addressed your comments

@lawrence-laz lawrence-laz merged commit 9d8fc35 into lawrence-laz:master Jul 24, 2023
1 check passed
@nicolaspierre1990
Copy link
Contributor Author

Will you push this to nuget ?

@lawrence-laz
Copy link
Owner

Will you push this to nuget ?

I wanted to make some unrelated changes as well. I'll try to get it done during my lunch break today.

@lawrence-laz
Copy link
Owner

The nuget is now pushed as 1.1.0, it will take a bit for it to pass validation but it should be listed publicly after an hour or so.

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.

None yet

2 participants