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.

Sign up for GitHub

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

Jump to bottom

Closes #3061 #3145

Merged
merged 48 commits into from
Feb 21, 2024
Merged

Closes #3061 #3145

merged 48 commits into from
Feb 21, 2024

Conversation

JustArchi
Copy link
Member

@JustArchi JustArchi commented Feb 12, 2024 •
edited
Loading

This is proof of concept for now, extensive testing and fixes are required before merging.

TODO:

  • Investigate how we can make [JsonDisallowNull] work
  • Test edge cases: [JsonExtensionData], JsonNode and JsonElement places
  • Do extensive testing otherwise

It seems to work good enough though to consider this for merging in the future.

Closes #3061

@JustArchi JustArchi added ✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 💣 Breaking changes Issues marked with this label indicate possible breaking changes that might need major version bump. 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. labels Feb 12, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 12, 2024 •
edited
Loading

Qodana for .NET

1 new problem were found

Inspection name Severity Problems
RoslynAnalyzers Do not catch general exception types ◽️ Notice 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@JustArchi JustArchi added 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🏁 Finished Issues marked with this label were finished already and no further work is required on them. and removed 🚧 Work in progress Issues marked with this label are in active work-in-progress and they're not ready for review yet. labels Feb 15, 2024
@JustArchi
Copy link
Member Author

@Rudokhvist @ezhevita @Abrynos this is now ready for review and/or testing if somebody wanted to.

Doc will arrive later, but copy-pasting my quick overview from Discord.

obraz

@nolddor
Copy link
Contributor

nolddor commented Feb 17, 2024

Thanks for all the effort you put on closing this one! <3

Abrynos
Abrynos approved these changes Feb 17, 2024
View reviewed changes
Copy link
Member

@Abrynos Abrynos left a comment

Choose a reason for hiding this comment

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

LGTM. Didn't do a full deep-dive just now, but I followed implementation as it was done and it looks alright scrolling through just now.

@JustArchi JustArchi merged commit 6b0bf0f into main Feb 21, 2024
44 checks passed
@JustArchi JustArchi deleted the feature/3061 branch February 21, 2024 02:09
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Abrynos Abrynos Abrynos approved these changes

Assignees
No one assigned
Labels
💣 Breaking changes Issues marked with this label indicate possible breaking changes that might need major version bump. ✨ Enhancement Issues marked with this label indicate further enhancements to the program, such as new features. 📢 Feedback welcome Issues marked with this label are open to any potential feedback that could help us. 🏁 Finished Issues marked with this label were finished already and no further work is required on them.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BREAKING] Investigate possibility of moving to System.Text.Json
3 participants
@JustArchi @nolddor @Abrynos