Skip to content
This repository was archived by the owner on Oct 4, 2021. It is now read-only.

Conversation

@nosami
Copy link
Member

@nosami nosami commented Sep 27, 2018

Consolidating ISystemInformation, UpdateInfoExtensionNode and
IUpdateInfoGenerator into ProductInformationProvider so that we only
have one way to retrieve product information.

Fixes VSTS #664616

Consolidating ISystemInformation, UpdateInfoExtensionNode and
IUpdateInfoGenerator into ProductInformationProvider so that we only
have one way to retrieve product information.

Fixes VSTS #664616
@nosami nosami force-pushed the telemetry-update-from-version branch from 853fd6e to 33c681c Compare September 27, 2018 10:50
Copy link
Member

Choose a reason for hiding this comment

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

Please move to MonoDevelop.Ide.Updater namespace and folder.

@nosami nosami force-pushed the telemetry-update-from-version branch from 33c681c to ff45acf Compare September 27, 2018 11:08

public override string ApplicationId => "c07628e8-5521-4c1a-aa3a-f860e664f0a9";

protected override string UpdateInfoFile => throw new NotImplementedException ();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this return null?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see, GetUpdateInfo overrules this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure. It's not used anywhere so I don't think it matters. I think throwing might be better, but happy to change it.

/// <summary>
/// Path to the updateinfo file.
/// </summary>
protected abstract string UpdateInfoFile { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we make this a virtual method that returns null? That way, implementors don't have to implement it with NotImplementedException.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reason behind this is that:

If we ever change UpdateInfoFile to be queried from anywhere else, the NIE will end up biting us.

{
}

public readonly string File;
Copy link
Contributor

Choose a reason for hiding this comment

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

To allow future changes, please make these readonly properties.

public string File { get; }

string url = null;
s = f.ReadLine ();
if (s != null) {
if (s.StartsWith ("source-url:"))
Copy link
Contributor

@Therzok Therzok Sep 27, 2018

Choose a reason for hiding this comment

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

introduce a constant for "source-url:".

const string sourceUrl = "source-url:";
if (s.StartsWith (sourceUrl)) {
    url = s.Substring (sourceUrl.Length).Trim ();
}

@nosami nosami force-pushed the telemetry-update-from-version branch from 76ecd87 to 6e31b86 Compare September 28, 2018 12:11

public override string Description => GetDescription ();

public override string Version => DotNetCoreSdk.Versions.FirstOrDefault().ToString ();
Copy link
Member

Choose a reason for hiding this comment

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

This should be either FirstOrDefault()?.ToString () or First().ToString ()

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered using System.Version for this, but Updater has a single digit version number that is too short.

I'll add the null check.

</Extension>

<Extension path = "/MonoDevelop/Core/SystemInformation">
<Class class = "MonoDevelop.Ide.RuntimeVersionInfo" />
Copy link
Member

Choose a reason for hiding this comment

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

You can register this class in the extension just above, no need to define a new one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good spot!

Copy link
Member

@slluis slluis 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!

@nosami nosami merged commit 8c7d3e2 into master Oct 2, 2018
@nosami nosami deleted the telemetry-update-from-version branch October 2, 2018 10:00
@nosami
Copy link
Member Author

nosami commented Oct 2, 2018

@monojenkins backport release-7.7

@nosami
Copy link
Member Author

nosami commented Oct 3, 2018

@monojenkins backport release-7.6

@monojenkins
Copy link
Contributor

@nosami backporting to release-7.6 failed, the patch results in conflicts:

Applying: Report _from_ version in addition to _to_ version
Using index info to reconstruct a base tree...
M	main/src/core/MonoDevelop.Ide/ExtensionModel/MonoDevelop.Ide.addin.xml
M	main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.csproj
.git/rebase-apply/patch:213: trailing whitespace.
// Copyright (c) 2018 
.git/rebase-apply/patch:287: trailing whitespace.
// Copyright (c) 2018 
warning: 2 lines add whitespace errors.
Falling back to patching base and 3-way merge...
Auto-merging main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.csproj
CONFLICT (content): Merge conflict in main/src/core/MonoDevelop.Ide/MonoDevelop.Ide.csproj
Auto-merging main/src/core/MonoDevelop.Ide/ExtensionModel/MonoDevelop.Ide.addin.xml
error: Failed to merge in the changes.
Patch failed at 0001 Report _from_ version in addition to _to_ version

Please backport manually!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants