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 Autoroute namespace #17457

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix Autoroute namespace #17457

wants to merge 2 commits into from

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Feb 4, 2025

I know this PR will introduce breaking changes, but we didn't use OC.xxx.Core in a namespace, it might be happened accidentally in the past so can we accept such a breaking change or shall I create a Core folder and move the old types then deprecate them?

@sebastienros @Piedone @MikeAlhayek please your feedback this is another major release that allow us to break things if it's possible, we already fixed many things in the past but I think there are few things need to be fixed before 3.0.0 to make the code consistent

@hishamco hishamco added the breaking change 💥 Issues or pull requests that introduces breaking change(s) label Feb 4, 2025
@hishamco
Copy link
Member Author

hishamco commented Feb 4, 2025

@sebastienros can we think about not relying on namespaces in YesSQL, I feel this like a gun shoot 🔫 when you make a mistake in a namespace, you can't backtrack

@MikeAlhayek
Copy link
Member

Based on our previous discussions, we agreed on the following rule:

Do not accept breaking changes solely because we are releasing a major version.

If a breaking change is introduced simply to add value—such as fixing a bug or improving functionality—we should reject it. However, if the benefits of the change outweigh the effort required, we may consider it.

With that in mind, renaming the AutorouteStateDocument class’s namespace is problematic, as it would require creating a new document with no existing records. While we could develop a migration to import the old records, the effort seems unnecessary. The change provides no real value beyond namespace consistency. Moreover, even if we resolve this inconsistency now, future restructuring could make it inconsistent again. This change does not align with the rule we established.

I encourage you to focus on addressing known bugs instead. Submitting PRs that fix existing issues would be far more valuable than those that merely rename or relocate types.

can we think about not relying on namespaces in YesSQL

Unless YesSQL provide a way to not depend on namespace, we have too work along with it. We can't just ignore namespace now because these are part of the identity of documents.

In short answer: I would suggest closing this PR.

@Piedone
Copy link
Member

Piedone commented Feb 5, 2025

Agree. Fixing the namespaces can be OK if it's just that and doesn't really bother consuming projects (because while public, the types are rarely referenced directly by consumers). But having to fix the document too, nah.

BTW the namespaces in the project is correct, so new classes added won't have this issue.

@sebastienros
Copy link
Member

As a side note, YesSql could be able to extract the serialized type name from an attribute. And also provide "providers" in options one being able to extract the attribute, or build it from the type itself (current implementation). But this wouldn't fix this breaking change.

@hishamco
Copy link
Member Author

hishamco commented Feb 5, 2025

I might agree with you guys that doing this will break, but I don't think other OSS will leave such a bug if it occurs accidently, I think major releases are steps to break, improve and make things consistent across the code-base, otherwise, all the known issues related the naming will not be fixed.

I encourage you to focus on addressing known bugs instead. Submitting PRs that fix existing issues would be far more valuable than those that merely rename or relocate types.

I know what you mean, I'm struggling to finalize my current PRs, then I could have a look into the unknown issues that I could fix them. BTW I noticed one of your PRs @MikeAlhayek relocating types in OC.Roles.Abstractions, which might good to make things consistent

As a side note, YesSql could be able to extract the serialized type name from an attribute. And also provide "providers" in options one being able to extract the attribute, or build it from the type itself (current implementation). But this wouldn't fix this breaking change.

Looks good

Yes, it will not fix this breaking change at the moment but will avoid such things in the future.

@hishamco
Copy link
Member Author

hishamco commented Feb 5, 2025

The question is what SHOULD we do in case we have a known inconsistent naming bug since 1.0.0? Shall we leave it as it's or we could have a plan to fix them

@MikeAlhayek
Copy link
Member

known inconsistent naming bug

Naming inconsistent naming is not bug. It just annoying naming inconsistency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change 💥 Issues or pull requests that introduces breaking change(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants