-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add WriteProgress and OpenProgress methods #38
base: master
Are you sure you want to change the base?
Conversation
Hi @halex2005! Thank you for the PR, we'll take a look at it this week. |
@@ -12,7 +12,7 @@ | |||
<GenerateAssemblyVersionAttribute>true</GenerateAssemblyVersionAttribute> | |||
<GenerateNeutralResourcesLanguageAttribute>true</GenerateNeutralResourcesLanguageAttribute> | |||
|
|||
<TargetFrameworks>netstandard1.3;net35;net40;net45;netcoreapp1.0;netcoreapp2.0;netcoreapp3.0</TargetFrameworks> | |||
<TargetFrameworks>netstandard1.3;netstandard2.0;net35;net40;net45;netcoreapp1.0;netcoreapp2.0;netcoreapp3.0;net6.0;net8.0</TargetFrameworks> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please share the reasoning behind adding new targets? As far as I understand, netstandard1.3
should cover all the frameworks that are covered by the added targets. Maybe some targets can even be removed, as netcoreapp1.0;netcoreapp2.0;netcoreapp3.0
seem redundant here.
I'm not sure what we should do with the target frameworks and I'm interested in hearing your opinion.
/// <summary> | ||
/// <para> | ||
/// Use <see cref="WriteProgress"/> method to add progress message | ||
/// <pre> | ||
/// ##teamcity[progressMessage '<message text>' ] | ||
/// </pre> | ||
/// </para> | ||
/// | ||
/// <para> | ||
/// Use <see cref="OpenProgress"/> method to add progress message | ||
/// <pre>##teamcity[progressStart '<message>']</pre> | ||
/// and | ||
/// <pre>##teamcity[progressFinish '<message>']</pre> | ||
/// on writer dispose. | ||
/// </para> | ||
/// <para> | ||
/// http://confluence.jetbrains.net/display/TCD18/Build+Script+Interaction+with+TeamCity#BuildScriptInteractionwithTeamCity-ReportingMessagesForBuildLog | ||
/// </para> | ||
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of remarks:
- Some minor grammatical changes will make the sentences sound better, e.g. "Use the method to add a progress message", "Use the method to add a progress message".
- For documentation, we should use a link to the latest version on https://www.jetbrains.com/help: https://www.jetbrains.com/help/teamcity/service-messages.html#Reporting+Build+Progress
I suggest something like this:
/// <summary> | |
/// <para> | |
/// Use <see cref="WriteProgress"/> method to add progress message | |
/// <pre> | |
/// ##teamcity[progressMessage '<message text>' ] | |
/// </pre> | |
/// </para> | |
/// | |
/// <para> | |
/// Use <see cref="OpenProgress"/> method to add progress message | |
/// <pre>##teamcity[progressStart '<message>']</pre> | |
/// and | |
/// <pre>##teamcity[progressFinish '<message>']</pre> | |
/// on writer dispose. | |
/// </para> | |
/// <para> | |
/// http://confluence.jetbrains.net/display/TCD18/Build+Script+Interaction+with+TeamCity#BuildScriptInteractionwithTeamCity-ReportingMessagesForBuildLog | |
/// </para> | |
/// </summary> | |
/// <summary> | |
/// <para> | |
/// Use the <see cref="WriteProgress"/> method to add a progress message: | |
/// <pre>##teamcity[progressMessage '<message text>' ]</pre> | |
/// </para> | |
/// | |
/// <para> | |
/// Use the <see cref="OpenProgress"/> method to add a progress start message: | |
/// <pre>##teamcity[progressStart '<message>']</pre> | |
/// and a | |
/// <pre>##teamcity[progressFinish '<message>']</pre> | |
/// progress finish message on writer dispose. | |
/// </para> | |
/// <para> | |
/// https://www.jetbrains.com/help/teamcity/service-messages.html#Reporting+Build+Progress | |
/// </para> | |
/// </summary> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, the documentation link in my previous comment was wrong. I have edited the comment to include the correct link.
public interface ITeamCityProgressWriter | ||
{ | ||
/// <summary> | ||
/// Writes normal message |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Writes a single progress message"?
void WriteProgress([NotNull] string message); | ||
|
||
/// <summary> | ||
/// Generates start flow message and returns disposable object to close flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about "Writes a progress start message and returns a disposable object that writes a progress finish message on dispose"?
Hi @halex2005! The implementation looks good to me, I just have a question regarding the target frameworks and some minor comments on wording. |
Hi!
According to build-script-interaction-with-teamcity.html#reporting-build-progress, there should be support of
and
service messages. This PR add this support.
Also I've noted that build targets somewhat obolete and I've added netstandard2.0, net6.0 and net8.0 targets.
I've also notices that WriteBuildProblem has required parameter named
identity
, bu documentation says thatidentity
should be optional:So, I've added additional method without
identity
parameter to not break compatibility with existing codePlease, review this PR.
Cheers, Aleksei.