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(dynamite): too long names for someOf typedefs #1350

Merged
merged 1 commit into from
Dec 25, 2023

Conversation

Leptopoda
Copy link
Member

No description provided.

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Ugh ok no we can't do this, it's a horrible user experience. We are just working around a problem with dart doc here, we need to fix it there.

@Leptopoda
Copy link
Member Author

What do you mean?

In theory we can make them visibleForTesting. Users would loose the ability to have a fromJson reviver but I doubt that this is a problem.
I also don't think that there is a way to fix this other than dartdoc generating html files with hashed names and using some sort of javaScript routing to handle the correct url client side. And I don't see them investing is such work.

@provokateurin
Copy link
Member

Well in some places one needs to create objects, e.g. spreed signaling. Using some weird hash name is not ok there

@Leptopoda
Copy link
Member Author

Well in some places one needs to create objects, e.g. spreed signaling. Using some weird hash name is not ok there

Please see the above code comment.

@Leptopoda Leptopoda force-pushed the feat/dynamite/long-class-names branch from 069a877 to a283eca Compare December 23, 2023 23:11
@provokateurin
Copy link
Member

Just to be sure I understand the approach: The serializers and records are private and hashed, but we have proxy extensions that are human readable. There might be multiple proxy extensions pointing to the same type as they would otherwise have to have the long names we had problems. Is this correct?

One problem I still see is that someone could create a schema that has >255 characters, but we can't do much about that and it would be quite stupid anyway.

@Leptopoda
Copy link
Member Author

One problem I still see is that someone could create a schema that has >255 characters, but we can't do much about that and it would be quite stupid anyway.

The good thing is that dynamite would not fail. They would just not be able to create documentation for their generated code (which is out of our responsibility).

@provokateurin
Copy link
Member

Can you confirm that I understand this change correctly?

@provokateurin
Copy link
Member

The commit message has a typo btw (too not to)

@Leptopoda
Copy link
Member Author

Ah sorry.
Yes you understand that correctly.

@Leptopoda
Copy link
Member Author

The commit message has a typo btw (too not to)

Is the rest fine? So I only need to push once more?

cspell.json Outdated Show resolved Hide resolved
cspell.json Outdated Show resolved Hide resolved
cspell.json Outdated Show resolved Hide resolved
packages/dynamite/dynamite/pubspec.yaml Outdated Show resolved Hide resolved
@Leptopoda Leptopoda force-pushed the feat/dynamite/long-class-names branch from a283eca to 1a616a2 Compare December 24, 2023 12:03
Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

Please fix the typo in the commit message and pr title before merging

refactor(dynamite): make someOf extensions and serializers private

Signed-off-by: Nikolas Rimikis <[email protected]>
@Leptopoda Leptopoda force-pushed the feat/dynamite/long-class-names branch from 1a616a2 to 75b27c7 Compare December 25, 2023 08:50
@Leptopoda
Copy link
Member Author

Can I get a new review?
I made the serializer getter private and added one per human readable name.
As it was static this would have been one (rare) case, where a dev might have encountered the hashed name.

@Leptopoda Leptopoda changed the title fix(dynamite): to long names for someOf typedefs fix(dynamite): too long names for someOf typedefs Dec 25, 2023
@Leptopoda Leptopoda enabled auto-merge December 25, 2023 12:49
@Leptopoda Leptopoda merged commit 54070e2 into main Dec 25, 2023
8 checks passed
@Leptopoda Leptopoda deleted the feat/dynamite/long-class-names branch December 25, 2023 12:49
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