Skip to content

Conversation

@Bashamega
Copy link
Contributor

@Bashamega Bashamega commented Dec 24, 2025

Also, I added full support for interface removals.
Everything now should be optionally nested except for the signature, because the emitter throws an error otherwise. So I had to add special handling for it in the converter

@github-actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@saschanaz
Copy link
Collaborator

saschanaz commented Dec 24, 2025

Also, I added full support for interface removals. Everything now should be optionally nested except for the signature, because the emitter throws an error otherwise. So I had to add special handling for it in the converter

Can you eleborate, like what kind of error? Working around an error is only good when we know what's happening there.

@Bashamega
Copy link
Contributor Author

Also, I added full support for interface removals. Everything now should be optionally nested except for the signature, because the emitter throws an error otherwise. So I had to add special handling for it in the converter

Can you eleborate, like what kind of error? Working around an error is only good when we know what's happening there.

Scratch that, I have updated the syntax

}
if (signatureObj.param?.length === 0 && !signatureObj.type) {
// If there are no params and no return type, remove the signature
signature = undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would cause problem for non-removal patches, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wouldn't cause an issue; we already have patches for non-removal methods, and it is working prefectly

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean if we want to add a signature... Hmm, but doing so would require a type, so maybe this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, so this is good. Anything else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not "good" but more like "I can live with this for now" for me... but yeah this should be okay.

Copy link
Collaborator

Choose a reason for hiding this comment

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

but constructors don't have a return type, so constructor is also a valid non-removal constructor. Doing this would break adding a parameter-less constructor 🤔

(Meaning we should throw if type exists for constructor btw)

Copy link
Contributor Author

@Bashamega Bashamega Dec 26, 2025

Choose a reason for hiding this comment

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

I will open another one of the throwing an error. It wouldn't count as a removal because the parameters array will be filled. We check for both

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove it when signatureIndex exists? Having signatureIndex without having params or types don't make sense.

(In that case we'll only remove the constructor/method's signature but not themselves, which I think is fine as it should ultimately behave same)

@Bashamega Bashamega requested a review from saschanaz December 26, 2025 04: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