-
Notifications
You must be signed in to change notification settings - Fork 380
[Core] Change libs to netstandard #687
base: master
Are you sure you want to change the base?
Conversation
Build on VS2017 works fine - It seems VS14 is hardcoded in appveyor - Please change it manually. |
@sochix Please check CI as it's useful pull request to merge |
You must seperate your commits : |
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.
Remove Unused Files
Hi, I've merged and drop unused files -> Merge should be OK - appveyor needs modification from repo owner (still hardlinked to VS14) |
I recive An item with the same key has already been added. Key: 481674261 when I use this dll's. :( |
Hi, what version you use ? This should be fixed with LORDofDOOM@cc09cb8 |
What's the motivation of migrating from 4.5.2 to 4.7? No Linux distributions have support for this .NET version yet. |
4.7 support netstandard reference 4.5.2 do not support this- If you use Linux use netcore |
That's wrong, I use .netStandard libraries all the time that allow both 4.5.x and .netStandard 2.0 usage (examples: NBitcoin and Nethereum).
Using .NET Core should be optional for TLSharp, not a requirement. You could improve this PullRequest to add support for .NET Core without the need to remove 4.5.x support. |
I'm trying to use this branch from .NET Core and get the following error:
Do you guys know which nuget package for DotNetZip (there's like a million of forks) can be used on .NET Core? |
Even if this work it's not supported - Here are official specifications - netstandard 2.0 is supported on 4.6.1 and greater - not on 4.5: https://docs.microsoft.com/en-US/dotnet/standard/net-standard
IMHO it is optional - Net 4.7 is available on every maintained Windows and if a user don't want to use netcore in Linux or unsupported Windows version (XP) he still can use mono -> It support 4.7: http://www.mono-project.com/docs/about-mono/compatibility/
This PR should work in netcore/netstandard: |
Good point, then please use 4.6.1, not 4.7.1.
That's only the very alpha versions. The mono versions that are included in Ubuntu LTS (16.04) and Debian 9, don't support 4.7 yet. Please change it to 4.6.1 and we're all happy. |
Could you please enlighten me how to reference DotNetZip so this PR doesn't crash? I downloaded the source code from the PR you referenced, opened a NetStandard solution and got Ionic.zip.NetStandard. After referencing that library, TLSharp still gave me an error about not being able to find Ionic.ZLip. I renamed Ionic.zip.NetStandard to Ionic.ZLib - still the same error message. |
I've changed full Framework to 4.6.1 :-)
Do you have any repo where I can check whats wrong ? What you have rename (Namespace or Filename) ? How you reference DotNetZip (dll or project) ? |
TeleSharp.TL/TLContext.cs
Outdated
select t).ToDictionary(x => ((TLObjectAttribute)x.GetCustomAttribute(typeof(TLObjectAttribute))).Constructor, x => x); | ||
Types.Add(481674261, typeof(TLVector<>)); | ||
} | ||
catch |
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.
What exception did you receive here? You should add the exception type, as putting "catch-all" blocks like this is quite dangerous.
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.
Sometime the error that @Jarzamendia describes top
I recive An item with the same key has already been added. Key: 481674261 when I use this dll's. :(
This happens if the assembly is loaded twice e.g. from any dependency injection. This only happens if 481674261 is already added - A global try/catch is OK here IMHO
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 global catch is never OK (the only exception is in the entry point of your application, for logging and bug-auto-submission purposes). When you're dealing with certain exception of type X, you need to use that type X in the code because the future developers need to know what was happening, what were you trying to fix. Please add the type.
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.
Add proper method to prevent double adding type
TLSharp.Tests.NUnit/packages.config
Outdated
@@ -1,4 +1,4 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<packages> | |||
<package id="NUnit" version="2.6.4" targetFramework="net45" /> | |||
<package id="NUnit" version="3.9.0" targetFramework="net47" /> |
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.
this is still pointing to 4.7
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.
Changed
TLSharp.Tests/app.config
Outdated
<add key="ApiHash" value=""/> | ||
<add key="ApiId" value=""/> | ||
<add key="NumberToAuthenticate" value=""/> | ||
<add key="CodeToAuthenticate" value=""/> |
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.
why do you change this? please remove this noise
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.
This is added from VS2017 because XML syntax is wrong in original file. There is no change here - Only correct spaces to MS defaults
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.
Well, you can remove this noise from the PullRequest. To not make this kind of mistake in the future, I recommend using a visualizing-tool for committing, for example git gui
. This way you know exactly what hunks you're committing.
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.
Changed to old file with spaces
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.
Changed
TeleSharp.Generator/App.config
Outdated
<configuration> | ||
<startup> | ||
<supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.5.2" /> | ||
<supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.7"/> |
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.
still pointing to 4.7, change it to 4.6.1 please
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.
Changed
TeleSharp.Generator/packages.config
Outdated
@@ -1,4 +1,4 @@ | |||
<?xml version="1.0" encoding="utf-8"?> | |||
<packages> | |||
<package id="Newtonsoft.Json" version="9.0.1" targetFramework="net452" /> | |||
<package id="Newtonsoft.Json" version="10.0.3" targetFramework="net47" /> |
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.
still pointing to 4.7
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.
Changed
<ItemGroup> | ||
<Compile Include="ObjectDeserializer.cs" /> | ||
<Compile Include="Properties\AssemblyInfo.cs" /> | ||
<Compile Include="TLContext.cs" /> |
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.
why are you removing all these files from this project?
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.
Because it's a new csproj format (VS17) and you don't need to explicitly include files. Everything inside project folder will be included automatically
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.
ok cool
TeleSharp.TL/TLContext.cs
Outdated
where t.GetCustomAttribute(typeof(TLObjectAttribute)) != null | ||
select t).ToDictionary(x => ((TLObjectAttribute)x.GetCustomAttribute(typeof(TLObjectAttribute))).Constructor, x => x); | ||
|
||
if (!Types.ContainsKey(481674261)) |
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.
ah I see what you did here, but actually I think the best approach is simply changing the line Types.Add(481674261, typeof(TLVector<>));
to simply Types[481674261] = typeof(TLVector<>);
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.
True -> This is a better method :) Changed
@knocte Any news on this? Its essential to add support of netcore to TLSharp |
I created a fork with netstandard and published it via nuget, it works in my project. |
Oops I thought that not all my comments had been addresses, so I was waiting for that. Now I'm not sure anymore if anything here is missing to do... But I've checked again at the diff and I have two more questions: why are some files like InitConnectionRequest.cs being removed? and why the upgrade of nunit from 2.x to 3.x? |
@knocte As you can see here, 'Requests' directory is legacy, so it was removed. Also I removed TLSharp.Tests.VS and inlined TLSharp.Tests.NUnit into TLSharp.Tests. I updated all packages, not just NUnit. I will not create a pull request for it, because I want to introduce subjective breaking changes, like changing transport classes hierarchy to disjoint unions. |
@knocte Also the main reason of my fork is absence of the owner for long time. |
Guys, I've made a big refactoring of the library finally, you can see the change list here. The code is much simpler and efficient than it was. The usability is much better because of discriminated unions. The transport will be updated to 82 layer soon. However, I struggle with one problem, ilyalatt#6. I do not have much of attempts to fix it, because after several logins I receive a flood error for a few hours. I will be grateful if somebody can help me to fix it. |
Make it run on Linux, rename unused files to *.cs.bak