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

Namespace unifications #300

Merged
merged 3 commits into from
Nov 12, 2021
Merged

Namespace unifications #300

merged 3 commits into from
Nov 12, 2021

Conversation

Simply007
Copy link
Contributor

Motivation

Fixes #287

Checklist

  • Code follows coding conventions held in this repo
  • Automated tests have been added
  • Tests are passing
  • Docs have been updated (if applicable)
  • Temporary settings (e.g. variables used during development and testing) have been reverted to defaults

How to test

If manual testing is required, what are the steps?

@Simply007 Simply007 requested a review from a team November 12, 2021 11:31
@Simply007 Simply007 mentioned this pull request Nov 12, 2021
5 tasks
@Simply007 Simply007 merged commit 184f41f into vNext Nov 12, 2021
@Simply007 Simply007 deleted the 284_dependency_issues branch November 12, 2021 12:05
@petrsvihlik
Copy link
Contributor

please don't do this...leaving abstractions in the root namespace was a conscious and intentional decision to make the whole API easily discoverable. also, you'll create a huge breaking change again + you'd need to update the code generator, etc.

https://github.com/Kentico/kontent-delivery-sdk-net/releases/tag/14.0.0

I'm already facing a problem because of this because I have dependencies targeting various versions of the library.
image

suggestion: revert this commit

@Simply007
Copy link
Contributor Author

please don't do this...leaving abstractions in the root namespace was a conscious and intentional decision to make the whole API easily discoverable. also, you'll create a huge breaking change again + you'd need to update the code generator, etc.

https://github.com/Kentico/kontent-delivery-sdk-net/releases/tag/14.0.0

I'm already facing a problem because of this because I have dependencies targeting various versions of the library. image

suggestion: revert this commit

Fair point!

I tried to find any official ruleset for namespaces and folders, but unsuccessfully. In unofficial content, opinions vary.

Could you elaborate more on your use case with multiple versions of the library that is causing the issues?

According to the generator's topic, the adjustments are planned. For the end project, it should be one automatic fix to set proper namespaces out of generated code.

In version 16, there will be a breaking change with the target framework with at least one namespace changes for Image transformation (#284), so this is a good time to clean up the namespace and set the best practices for code.

I will discuss the pros and cons of this change with maintainers on Thursday (2021-11-18).


Saving the commit ID for future reference: c5ea204

@petrsvihlik
Copy link
Contributor

Could you elaborate more on your use case with multiple versions of the library that is causing the issues?

Yeah, it's caused by referencing both Kentico.Kontent.Delivery and Kontent.Statiq (which references K.K.Delivery too) in my project. So any API breaking change between the two K.K.D. referenced is causing the errors.

so this is a good time to clean up the namespace and set the best practices for code.

Yeah, that's what we said the last time when we decided to unify it this way. :)

Anyways, if you decide to go this path, I'd really suggest making the generator part of this repo and releasing it always together with fresh nugets.

@Simply007
Copy link
Contributor Author

Yeah, that's what we said the last time when we decided to unify it this way. :)

We will stick with the original approach. I will revert the commit c5ea204 and add information about namespaces into README at least so that we won't do go this way again.

Anyways, if you decide to go this path, I'd really suggest making the generator part of this repo and releasing it always together with fresh nugets.

Let's leave this decision for #249

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.

4 participants