Skip to content
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

[feature/constraints] Template constraints proposal #4593

Merged
merged 1 commit into from
May 20, 2022

Conversation

vlada-shubina
Copy link
Member

Problem

#3107
template constraints feature proposal

@vlada-shubina vlada-shubina force-pushed the constraints-design branch 2 times, most recently from 5a060a4 to 37b90eb Compare April 18, 2022 12:55
@vlada-shubina vlada-shubina marked this pull request as ready for review April 18, 2022 12:57
@vlada-shubina vlada-shubina requested review from a team and baronfel as code owners April 18, 2022 12:57
@vlada-shubina vlada-shubina changed the title [feature/constraints][draft] Template constraints proposal [feature/constraints] Template constraints proposal Apr 18, 2022
@vlada-shubina
Copy link
Member Author

#4606 may help with the review, it contains Abstractions and API definitions.

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added few minor comments for consideration


`dotnet new` will implement the following changes based on constraints:
- the template which is not allowed in the context will not be shown in `dotnet new list` output
- additional option (`--force`) may be considered to show all the templates anyway
Copy link
Member

Choose a reason for hiding this comment

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

Plus it might be good idea for the debug output to contain the context of the constraint evaluated (value of variables) - for easier troubleshooting.

Copy link
Member Author

Choose a reason for hiding this comment

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

would it be more useful to output it always in verbose mode? (basically output it in debug output which is shown in verbose mode)

```
When creating the constraint, it is assumed that constraint will be initialized and its context will be evaluated.
Example: if constraint needs to know the project MSBuild properties it is assumed that project will be identified and MSBuild properties will be evaluated when the constraint is being created prior to first evaluation.
Template constraints initialization will be started in parallel once `IEngineEnvironmentSettings` are created. The methods that work with constraint will ensure task completion before evaluation.
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the Task is not yet complete when evaluation is already called? Will it block untill constraint is fully initialized? This can probably create suboptimal experience with tab completion. For this case we might want to consider adding a mechanism to wait for predefined timespan for the constraint to be intialized (e.g. Task<bool> WaitForInitialization(TimeSpan span)). Tab completition might then wait just for a brief delay and then consider the constraint not to be met (or otherwise - depending on desired behavior).

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, correctly. Methods of TemplateConstraintManager - the class to be used when working with constraints - will wait until required constraint is initialized.
It makes total sense to add a timeout for constraint evaluation for tab completion and only return information about those constraints that could be initialized. I think it might just be an additional parameter to methods of TemplateConstraintManager.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking more (and implementation wise) it's up to a caller to define the timeout. So, it didn't make it to API.

Copy link
Contributor

@bekir-ozturk bekir-ozturk left a comment

Choose a reason for hiding this comment

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

Looks great! And thanks for taking the time to putting this in writing.

Aside from my comments, I have one question:
Should we make a clear decision on whether constraint initializations can be executed in parallel?
If yes, we will need to be more careful when implementing new constraints, but we may get some nice performance benefits in the long run.
If we decide that they will be executed in a serialized way, it may be difficult to make it parallelizable later. So, this may be a good time to consider pros and cons.

@vlada-shubina vlada-shubina changed the base branch from feature/constraints to main May 17, 2022 10:53
@vlada-shubina vlada-shubina enabled auto-merge (rebase) May 19, 2022 10:27
@vlada-shubina vlada-shubina merged commit eab7324 into dotnet:main May 20, 2022
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