Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

C#: Default subtypes to true. #18060

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

Draft
wants to merge 3 commits into
base: main
Choose a base branch
Loading
from

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Nov 21, 2024

No description provided.

@github-actions github-actions bot added the C# label Nov 21, 2024
Copy link
Contributor

⚠️ The head of this PR and the base branch were compared for differences in the framework coverage reports. The generated reports are available in the artifacts of this workflow run. The differences will be picked up by the nightly job after the PR gets merged.

Click to show differences in coverage

csharp

Generated file changes for csharp

  • Changes to framework-coverage-csharp.rst:
-    System,"``System.*``, ``System``",47,10818,54,5
+    System,"``System.*``, ``System``",47,10817,54,5
-    Totals,,104,12893,396,5
+    Totals,,104,12892,396,5
  • Changes to framework-coverage-csharp.csv:
- System,54,47,10818,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5511,5307
+ System,54,47,10817,,6,5,5,,,4,1,,33,2,,6,15,17,4,3,,5511,5306

@michaelnebel
Copy link
Contributor Author

@owen-mc : Didn't report any changes.

@owen-mc
Copy link
Contributor

owen-mc commented Nov 22, 2024

Interesting. Do you think it's a feature that isn't used much? Or is it possible that inheritance isn't quite working as intended?

@michaelnebel
Copy link
Contributor Author

Interesting. Do you think it's a feature that isn't used much? Or is it possible that inheritance isn't quite working as intended?

I think it works as intended, but maybe many of the summaries that already were relevant for subtypes = true had that set. The FlowSummaries test also reports that changing the logic only means that all the summaries applies to 44 extra callables (in all the libraries/frameworks included in the test) - so it is not that surprising that we don't see any changes in the alerts.

@michaelnebel
Copy link
Contributor Author

@michaelnebel
Copy link
Contributor Author

@owen-mc : The QA dashboard only shows a few alert changes, so I don't think it will have a big effect for C# if we set the subtypes = true as default.

@owen-mc
Copy link
Contributor

owen-mc commented Nov 25, 2024

Great. Out of interest, did you check those alerts to see if you think they are valid?

@michaelnebel
Copy link
Contributor Author

Great. Out of interest, did you check those alerts to see if you think they are valid?

No, I did not :-)

@michaelnebel
Copy link
Contributor Author

@owen-mc : Do we want to merge this PR or do you prefer to do this for all languages in the same PR?

@owen-mc
Copy link
Contributor

owen-mc commented Dec 9, 2024

@michaelnebel I'm happy for you to merge it. I'm not sure when I'll get around to working on this.

@michaelnebel
Copy link
Contributor Author

michaelnebel commented Dec 9, 2024

@michaelnebel I'm happy for you to merge it. I'm not sure when I'll get around to working on this.

Ok, I think it makes sense to merge, if we do the same thing in conjunction with Java.
Is there a similar Java PR?
If we do so, we also need to update the MaD documentation.

@owen-mc
Copy link
Contributor

owen-mc commented Dec 9, 2024

Oops, I forgot that this PR wasn't just changing the "false" to "true" in all the models. Yes, in that case we should do something more coordinated between at least the static languages. And see if the dynamic languages have an equivalent.

@michaelnebel
Copy link
Contributor Author

Ah, so you merged a PR that changes the override/subtyping from false -> true for all Java models?

@owen-mc
Copy link
Contributor

owen-mc commented Dec 10, 2024

No, I hadn't got round to thinking about it yet. And I had forgotten that you had already done all the hard work here, and we just have to copy it for other languages. For Go I manually changed all the models to use "True" where it has any meaning, and "False" when it doesn't have a meaning (I wanted to use "True" for all but others disagreed). I haven't done anything for Java.

@michaelnebel
Copy link
Contributor Author

Alright! Then let us leave it for now. It doesn't seem "urgent", so we can postpone - but hopefully not indefinitely! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.