Skip to content

Expose Properties and Items data enumeration #10771

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

Merged
merged 17 commits into from
Oct 31, 2024

Conversation

JanKrivanek
Copy link
Member

@JanKrivanek JanKrivanek commented Oct 8, 2024

Fixes #10770

Context

The ProjectEvaluationFinishedEventArgs and ProjectStartedEventArgs Properties and Items members are nongeneric and need involved custom code to enumerate.
While we do not want to expose exact internals to prevent ourselves from future changes - we should at least expose the way how the data can be traversed in string form.

Previous state

Users needed to recreate EnumerateItems and EnumerateProperties methods, which calls other internal helper methods and together use several internal types (ItemDictionary, IItem, IProperty, IKeyed, etc.) - so some of the cases need to be skipped by the custome code, and hence despite involved, the code is usually single purpose.

Sample of that can be found e.g. here:

https://devdiv.visualstudio.com/DevDiv/_git/vs-code-coverage/pullrequest/582161?path=/src/cts/Build/BinlogReader.cs&version=GBmain&line=108&lineEnd=128&lineStartColumn=1&lineEndColumn=20&type=2&lineStyle=plain&_a=files&iteration=11&base=0

with

https://devdiv.visualstudio.com/DevDiv/_git/vs-code-coverage/pullrequest/582161?path=/src/cts/Build/MSBuildHelper.cs&version=GBmain&line=36&lineEnd=60&lineStartColumn=1&lineEndColumn=6&type=2&lineStyle=plain&_a=files&iteration=11&base=0

So something like:

        // the helper needs to be defined as well - attempting various casting. Similar to the internal EnumerateItems linked above
        MSBuildHelper.EnumerateItems(projectEvaluationFinishedEventArgs.Items, entry =>
        {
            if (entry.Key.Equals(ProjectReferencePropertyName))
            {
                if (entry.Value is ITaskItem)
                {
                    string referenceProjectPath = ((ITaskItem)entry.Value).ItemSpec;

                    // ...
                }
            }
        });

Proposed

The discussed code would change to:

        foreach (ItemData itemData in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = itemData.EvaluatedInclude;
            IEnumerable<KeyValuePair<string, string>> metadata = itemData.EnumerateMetadata();
            // ...
        }

The enumeration is as well demonstrated by tailored unit tests: src/Build.UnitTests/BuildEventArgsDataEnumeration.cs

Notes

The decision on extension methods vs. member methods

In some cases (e.g. the EnumerateMetadata and GetEvaluatedInclude for ITemData) it was necessity not to break backward compatibility of interfaces. Nor could I introduce new interfaces and return wrapped types - as some of the existing consumers (including our code) relies on casting of enumerated data to specific types. Wrapping the data would break this.

As for the EnumerateItems and EnumerateProperties methods for data on BuildEventArgs - I have no strong opinions on this, and ready to flip to member methods if it seems better fit.

Testing

  • Added enumeration unit tests
  • Manual (via project the prototype project that cosumed the data)

FYI @jakubch1

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Can you write up some motivation for doing it with extension methods, and what an old/new calling pattern would be?

@JanKrivanek
Copy link
Member Author

Can you write up some motivation for doing it with extension methods, and what an old/new calling pattern would be?

@rainersigwald I've added a motivation + sample to the description - please have a look if it makes sense.

As for 'extension vs member methods' - no strong opinions :-) it can be changed easily if it feels more proper.

@JanKrivanek JanKrivanek force-pushed the proto/expose-eventargs-enumeration branch from d2bd335 to 080bf5a Compare October 11, 2024 09:58
Copy link

@donJoseLuis donJoseLuis left a comment

Choose a reason for hiding this comment

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

very minor comments

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

Is this what we want to expose, or should it be something more like what we expose on Project{Instance}, such as ProjectInstance.GetItems(string type)?

@JanKrivanek
Copy link
Member Author

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

Is this what we want to expose, or should it be something more like what we expose on Project{Instance}, such as ProjectInstance.GetItems(string type)?

I'd prefer to avoid that - while already public, those types are R/W. So let's expose a common base of those types - that still gives read access to important data (EvaluatedInclude, Metadata a string-string map) - which is more than what's made available today, and we can allways expose more if needed.

What do you think?

@JanKrivanek
Copy link
Member Author

The discussed code would change to:

        foreach ((string itemType, IItemData itemValue) tuple in projectEvaluationFinishedEventArgs
                     .EnumerateItems()
                     .Where(i => i.itemType.Equals(ProjectReferencePropertyName)))
        {
            string referenceProjectPath = tuple.itemValue.GetEvaluatedInclude();
            // ...
        }

Is this what we want to expose, or should it be something more like what we expose on Project{Instance}, such as ProjectInstance.GetItems(string type)?

I'd prefer to avoid that - while already public, those types are R/W. So let's expose a common base of those types - that still gives read access to important data (EvaluatedInclude, Metadata a string-string map) - which is more than what's made available today, and we can allways expose more if needed.

What do you think?

Actualy it's even more complicated - we pass only ProjectItem and ProjectItemInstance to the EventArgs during creation, but on node-2-node translation and binlog de/serialization those can be changed to TaskItemData. So without breaking existing public API we do not have any common type hierarchy. At the same type we cannot wrap - not to break consumers that cast the items.

So the way around is via an accessor object (something like flyweight pattern) - the target object is still just original object and can be casted, but the iterator provides accessor methods for strong-type-like accessing EvaluatedInclude and metadata.

@JanKrivanek JanKrivanek enabled auto-merge (squash) October 31, 2024 12:49
@JanKrivanek JanKrivanek merged commit 2b126ff into main Oct 31, 2024
10 checks passed
@JanKrivanek JanKrivanek deleted the proto/expose-eventargs-enumeration branch October 31, 2024 13:12
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.

API exposure for Test CTS
6 participants