Skip to content

Clean up record_representation type #6957

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

Conversation

DZakh
Copy link
Member

@DZakh DZakh commented Aug 17, 2024

Yet another clean up PR to better familiarize with the codebase

@cristianoc
Copy link
Collaborator

I think changes to the internal representation of types might be problematic for the editor extension (which vendors the compiler).
@zth right?

@cknitt
Copy link
Member

cknitt commented Aug 17, 2024

Is the has_optional really needed or couldn't we just match on the optional_labels themselves`?
I am worried it might lead to inconsistent state.

@DZakh
Copy link
Member Author

DZakh commented Aug 18, 2024

I think changes to the internal representation of types might be problematic for the editor extension (which vendors the compiler).

Could you explain which changes are acceptable and how it is related to the editor extension?

@DZakh
Copy link
Member Author

DZakh commented Aug 18, 2024

Is the has_optional really needed or couldn't we just match on the optional_labels themselves`? I am worried it might lead to inconsistent state.

Hmmm, I think then it's better to keep the type as it was before :)

@cristianoc
Copy link
Collaborator

I think changes to the internal representation of types might be problematic for the editor extension (which vendors the compiler).

Could you explain which changes are acceptable and how it is related to the editor extension?

See 0a60758#diff-bf8a7b89f35f42262078976d00e99dd1aa6e3bbd7b981b44d48cf50a31af9541

@zth
Copy link
Collaborator

zth commented Aug 18, 2024

I think changes to the internal representation of types might be problematic for the editor extension (which vendors the compiler). @zth right?

Correct. I replied in the other thread too, but in short - we can't change the runtime representation as of now before we have tooling that's versioned with and tied to the compiler version itself. Which isn't the case right now.

As for what would change runtime representation, @cristianoc knows that much better than me.

@DZakh
Copy link
Member Author

DZakh commented Aug 19, 2024

It's not relevant then. Even though I close the PR, an explanation on what changes the runtime representation would help

@DZakh DZakh closed this Aug 19, 2024
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