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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

DotNet core support, VS2017 migration / csproj work etal #331

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@ChrisMcKee
Contributor

ChrisMcKee commented Mar 15, 2017

This should probably be a major version change; but all in all its pretty much backwards compatible (bar 6581d0c which brings naming in line with c#.net std).

I've ran the tests manually against a test instance of Algolia; the travis config will be wrong and will need rejigging (Fixing codes one thing, your CI environments yours 馃憤 ) .


e7b1fb3

6581d0c

*Change naming to match c# conventions (all a tad mixed) / correct typos and missing xmldoc / correct enumeration

  • _dsnInternalTimeout is styled as if private; when public and publically set in the tests
  • CT Token taken in but not passed along in WaitTaskAsync, time to wait also missing in xml doc (necessary as its int not timespan it could be anything)
  • use base types where possible (string not String)
  • in DeleteObjectsAsync enumerable taken in is string, foreach type should be string not object.
  • access before async (public async not the more javsacript looking async public)
  • weirdly manual test for null or empty on .net string type, cursor != null && cursor.Length > 0 gains no optimization.
  • use of ienumerable in SearchDisjunctiveFacetingAsync, foreach will enumerate, contains will enumerate again; converted to array though list would of been fine.

In case you want to look at dropping PCL in the existing branch the code should look like...

#if WINDOWS_RT
    using Windows.Security.Cryptography;
    using Windows.Security.Cryptography.Core;
    using Windows.Storage.Streams;
#else
    using System;
    using System.Security.Cryptography;
    using System.Text;
#endif

    internal static class CryptoUtility
    {
        internal static string ComputeHmac256(string key, string message)
        {
#if WINDOWS_RT
            var keyMaterialBytes = StringToAscii(key);
            MacAlgorithmProvider macAlgorithmProvider = MacAlgorithmProvider.OpenAlgorithm("HMAC_SHA256");
            IBuffer keyMaterial = CryptographicBuffer.CreateFromByteArray(keyMaterialBytes);
            CryptographicKey hmacKey = macAlgorithmProvider.CreateKey(keyMaterial);
            IBuffer messageBuffer = CryptographicBuffer.ConvertStringToBinary(message, BinaryStringEncoding.Utf8);
            IBuffer signedMessage = CryptographicEngine.Sign(hmacKey, messageBuffer);
            return signedMessage.Aggregate("", (s, e) => s + String.Format("{0:x2}", e), s => s);
#else
            using (HashAlgorithm hashAlgorithm = new HMACSHA256(key))
            {
              string hash;
              ASCIIEncoding encoder = new ASCIIEncoding();
              Byte[] code = encoder.GetBytes(key);
              using (HMACSHA256 hmac = new HMACSHA256(code))
              {
                  Byte[] hmBytes = hmac.ComputeHash(encoder.GetBytes(data));
                  hash = hmBytes.Aggregate("", (s, e) => s + String.Format("{0:x2}", e), s => s);
              }
              return hash;
            }
#endif
        }
    }

ChrisMcKee added some commits Mar 15, 2017

Resolves #259; Add support for .net core / standard. Remove reliance 鈥
鈥n PCL. Drop depreciated targets...

* Set target frameworks in csproj; remove depreciated platforms https://docs.microsoft.com/en-us/nuget/schema/target-frameworks
* Set nuget defaults based on current algolia csharp on nuget site; you can see the properties here https://docs.microsoft.com/en-us/nuget/schema/msbuild-targets
* Drop PCLCrypto , at a wild guess this was 5 years ago to support winrt; as it stands it could have been handled in a single helper class using compiler tags.
 * https://msdn.microsoft.com/en-us/library/system.security.cryptography.hmacsha256(v=vs.110).aspx
* Remove TaskEx (not been a valid class since 2012 CTP)
 * https://msdn.microsoft.com/en-us/library/system.threading.tasks.task(v=vs.110).aspx
* Correct framework in tests; generally use your own assembly namespace, not NUnits
* Swap auth key / api key test values for ones from python, lifes easier when all the clients play together. (TestGenerateSecuredApiKey)
* Swap "脿lgol?脿-csharp for plain text; if this is supposed to test UTF-8 or similar then use test cases and a specific test to create/destroy indexes
Change naming to match c# conventions (all a tad mixed) / correct typ鈥
鈥s and missing xmldoc / correct enumeration

* _dsnInternalTimeout is styled as if private; when public and publically set in the tests
* CT Token taken in but not passed along in WaitTaskAsync, time to wait also missing in xml doc (necessary as its int not timespan it could be anything)
* use base types where possible (string not String)
* in DeleteObjectsAsync enumerable taken in is string, foreach type should be string not object.
* access before async (public async not the more javsacript looking async public)
* weirdly manual test for null or empty on .net string type,  `cursor != null && cursor.Length > 0` gains no optimization.
* use of ienumerable in SearchDisjunctiveFacetingAsync, foreach will enumerate, contains will enumerate again; converted to array though list would of been fine.
@CBaptiste

This comment has been minimized.

Member

CBaptiste commented Mar 16, 2017

Wow.Impressive. Looking into it right now !!

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented Mar 27, 2017

so far so good, it builds and test are successful

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented Mar 27, 2017

@CBaptiste cool; I don't like making PRs that set the world on fire but it seemed the only way around this. Joys of modern .net 馃榿

@CBaptiste

This comment has been minimized.

Member

CBaptiste commented Apr 18, 2017

hey @ChrisMcKee so sorry for the delay :(
I intent to finalize this PR this week - it took more time than expected \o/

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented Apr 21, 2017

Cool; guess its all linked in to the core / #333
Once this is in, can look at adding a slightly higherlevel api over the top of the existing methods to reduce all the jobject fun

@ChrisMcKee

This comment has been minimized.

Contributor

ChrisMcKee commented Apr 21, 2017

@CBaptiste one downside to the test suite in this; if I run it a couple of times I wipe out a free account >< might need to look at altering how those run later ;)

I expect to have more prs as I deal with using this to load in our millions of docs / updates / removals daily as I'll be benchmarking at the same time. It'll get a good shaking, see what falls out 馃挘

@CBaptiste CBaptiste closed this May 19, 2017

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