Skip to content

chore: readonly not suppose to have default value#2180

Merged
bwoebi merged 3 commits into
DataDog:masterfrom
Ferror:stubs
Aug 17, 2023
Merged

chore: readonly not suppose to have default value#2180
bwoebi merged 3 commits into
DataDog:masterfrom
Ferror:stubs

Conversation

@Ferror

@Ferror Ferror commented Jul 29, 2023

Copy link
Copy Markdown
Contributor

I wanted to use the stubs, and it seems that not all are defined correctly.

Btw. why not to move them to https://github.com/JetBrains/phpstorm-stubs?

@Ferror Ferror requested a review from a team as a code owner July 29, 2023 15:52
Comment thread ext/ddtrace.stub.php
/**
* @var SpanStack|null The parent stack, or 'null' if there is none
*/
public readonly SpanStack|null $parent = null;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a reason you removed that readonly here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes. You may notice that like 153 defines whole class as a readonly, so it's invalid to duplicate it to property too.

@bwoebi

bwoebi commented Jul 31, 2023

Copy link
Copy Markdown
Collaborator

@Ferror We need these stubs in our own repository as the arginfo.h are generated from these.

But good finds that they cannot have a default value.

@Ferror Ferror requested a review from bwoebi August 7, 2023 18:56
@bwoebi

bwoebi commented Aug 8, 2023

Copy link
Copy Markdown
Collaborator

I'll merge it next week, have to also regenerate stubs and test whether it then still works properly.

@bwoebi bwoebi left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

SpanStack itself is not supposed to be readonly, that was a mistake, removed it there instead.

@bwoebi bwoebi merged commit 378af52 into DataDog:master Aug 17, 2023
@Ferror Ferror deleted the stubs branch August 19, 2023 19:37
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