Deep link parsers and tests #368

brandonh-msft · 2016-09-22T17:55:39Z

The purpose of this PR is to provide UWP devs an easy way to deal with Deep link URIs as they become more and more a part of the services we create for devs to consume. Bot Framework, Cortana launches, etc all rely on deep links in to apps to ultimately provide value.

DeepLinkParser

This class provides a way to create, from IActivatedEventArgs a Dictionary<string,string>-inheriting object that provides an additional .Root property to pull the base path of the URI (eg: in MainPage/Options?option1=value1, .Root would be MainPage/Options.
Once you have an instance, simply saying instance["optionName"] will pull the value from the querystring for that option.

CollectionFormingDeepLinkParser

Some consumers want to be able to do something like ?pref=this&pref=that&pref=theOther and have a pull of pref come back with this,that,theOther as its value. This derivative of DeepLinkParser provides this functionality.

Both of these are createable using a .Create(IActivatedEventArgs) method. Should a user wish to create one in a different manner, the default constructor is protected so inheriting from either of these can provide extensibility should they wish to pass in just a string, etc. The method that does the heavy lifting of parsing in to the Dictionary<string,string> is also protected so can be used by any inheriting class.

msftclas · 2016-09-22T17:55:42Z

Hi @brandonh-msft, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Brandon Hurlburt). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

ScottIsAFool · 2016-09-22T18:04:54Z

Microsoft.Toolkit.Uwp/Helpers/DeepLinkParser.cs

+                    .Select(param =>
+                    {
+                        var kvp = param.Split('=');
+                        return new KeyValuePair<string, string>(kvp[0], kvp[1]);


You've essentially done this conversion to KeyValuePair list twice now, could we move this to a helper method instead?

ScottIsAFool · 2016-09-22T18:07:25Z

Microsoft.Toolkit.Uwp/Helpers/DeepLinkParser.cs

+            var origString = validatedUri.OriginalString;
+            int queryStartPosition = origString.IndexOf('?');
+            if (queryStartPosition == -1)
+            { // No querystring on the URI


I think style suggests that comments should be on the next line.

ScottIsAFool · 2016-09-22T18:07:34Z

Microsoft.Toolkit.Uwp/Helpers/DeepLinkParser.cs

+            int queryStartPosition = origString.IndexOf('?');
+            if (queryStartPosition == -1)
+            { // No querystring on the URI
+                this.Root = origString;


Remove usages of this.

deltakosh · 2016-10-05T18:33:43Z

up

brandonh-msft · 2016-10-05T20:57:05Z

@deltakosh i submitted a new push, should be able to merge it in no problem.

IbraheemOsama · 2016-10-06T03:49:12Z

@brandonh-msft it seems like there are conflicts that must be resolved.

IbraheemOsama · 2016-10-06T03:51:46Z

UnitTests/DeepLinkParserTests.cs

+    {
+        public TestContext TestContext { get; set; }
+
+        private const string SAMPLELINK = @"MainPage/Options?option1=value1&option2=value2&option3=value3";


I guess this should be SampleLink not SAMPLELINK

IbraheemOsama · 2016-10-06T03:57:31Z

UnitTests/DeepLinkParserTests.cs

+        public TestContext TestContext { get; set; }
+
+        private const string SAMPLELINK = @"MainPage/Options?option1=value1&option2=value2&option3=value3";
+        private static readonly DeepLinkParser _parser = new TestDeepLinkParser(SAMPLELINK);


Also _parser should be Parser

IbraheemOsama · 2016-10-06T03:59:03Z

UnitTests/DeepLinkParserTests.cs

+    public class CollectionCapableDeepLinkParserTests
+#pragma warning restore SA1402 // File may only contain a single class
+    {
+        private const string SAMPLELINK = @"MainPage/Options?option1=value1&option2=value2&option3=value3&option2=value4";


SAMPLELINK Should be SampleLink too

IbraheemOsama · 2016-10-06T03:59:17Z

UnitTests/DeepLinkParserTests.cs

+#pragma warning restore SA1402 // File may only contain a single class
+    {
+        private const string SAMPLELINK = @"MainPage/Options?option1=value1&option2=value2&option3=value3&option2=value4";
+        private static readonly DeepLinkParser _parser = new TestCollectionCapableDeepLinkParser(SAMPLELINK);


_parser should be Parser

lukasf · 2016-10-24T14:15:43Z

Is there a specific reason why you do not open this up for use with normal string or uri arguments? Parsing options from uri strings is a very common scenario, not only useful for deeplinks but also all other kinds of links.

Even when only using this for deep links, it should be opened up for string arguments as well: I am pretty sure that in almost all apps, the string will be passed to a frame's navigate function. So in the app class, only the .Root will be checked to get the page type. Only later in the page's navigate function, the user will actually want to parse out the parameters from the string argument. So a DeepLinkParser which only works with IActivatedEventArgs would be pretty useless for most apps, I think. I would propose to at least give it a public string overload. And if you ask me, I'd even rename it to something like LinkParser or LinkQueryParser and add an Uri overload as well, to make it truly universal.

brandonh-msft · 2016-10-24T15:06:16Z

@lukasf
Deep links launch in to the Activated method, which has the deep link as part of the IActivatedEventArgs parameter.
Note that the construct is not sealed. Creating one that takes a string would be trivial should someone deem that necessary in their project.

deltakosh · 2016-10-24T17:37:13Z

Do you mind fixing conflicts and checking @IbraheemOsama ask?

brandonh-msft · 2016-10-24T18:34:18Z

@deltakosh
since @IbraheemOsama's asks were all stylistic, are there some more/other analyzers we can apply/enforce here?

brandonh-msft · 2016-10-24T18:59:41Z

@lukasf you might want to check out a branch i already had in my fork but haven't yet updated to the latest code if you're interested in this approach.
Forgot i'd played w/ just such a solution as a way to feel out the extensibility :)

lukasf · 2016-10-24T20:24:07Z

@brandonh-msft My point is, the vast majority of apps use frame navigation, they will need to have the link options accessible in the page's OnNavigatedTo() function. It does not help much to have the link options available in the App class, when there is no way to pass complex objects (such as the DeepLinkParser, Dictionaries, etc) through there without breaking Frame's navigation state serialization. So the easiest way for all these apps to work with deep links is to pass the deep link as string parameter to Frame.Navigate() and then parse out the options in Page.OnNavigatedTo(). The goal of this toolkit is to simplify things for developers. It would be very unfortunate if nearly all app devs would need to inherit DeepLinkParser, only to successfully use it with frame navigation apps.

brandonh-msft · 2016-10-24T20:31:57Z

@lukasf
I understand what you're saying. Links passed to pages aren't "deep links" - they're parameter values. The app-level Activated method still needs to hit that content then kick it off to the page. At which point the Activated method could send them the URI string that came in (from which DeepLinkParser was created) or the raw string, or a few values that were pulled out by the parser. I wouldn't expect a parser to be sent to a page. I'd expect a value to.

Regardless, i'll work on shoring up the string-capable version of this PR and add it in.

lukasf · 2016-10-25T08:18:10Z

@brandonh-msft Normally I would fully agree with you. But unfortunately there is this restriction that the parameter passed to the Page through Frame.Navigate can only be one of the simple types string, numeric, char or Guid. No complex values are allowed, no dicts or arrays. So passing data as a single string is the only feasible way. If there is no string overload in DeepLinkParser, devs would have to serialize the parsed options again in the app and then deserialize in the page. With string overload, they can just move the options parsing right into the page and save the whole serialize/deserialize. I am sure that this will be very useful to a lot of devs.

ScottIsAFool · 2016-10-25T08:21:13Z

Eh? That's simply not true. Frame.Navigate can have anything passed through, I do it all the time.

lukasf · 2016-10-25T08:32:40Z

@ScottIsAFool Well right, you can pass anything, but then the next call to GetNavigationState() will fail with an exception. If you want to persist the frame naviagation state on app suspend/resume, you cannot pass complex values. This is also mentioned in the msdn docs for Frame.Navigate().

ScottIsAFool · 2016-10-25T08:34:57Z

@lukasf right, you made no comment about this originally, hence the confusion :) thanks for clarifying.

lukasf · 2016-10-25T08:42:39Z

@ScottIsAFool Sure, no worries. :) Sorry for the confusion.

brandonh-msft · 2016-10-25T16:31:43Z

@lukasf
Please note that the code includes string overloads as of yesterday. need to figure out why build works on my box but fails in appveyor now.

deltakosh · 2016-10-26T22:30:38Z

Conflicts :( Sorry should be because of last merges)

nice exception for repeated keys when using DeepLinkParser more meaningful var name more single-char var name fixes xml doco fix Uri & string overloads along with UTs

…ng of query parameters from a string or Uri in to a collection that can be iterated on

adding StyleCop analyzers to UT project Stylecop ignoring as did review call stack.

deltakosh · 2016-10-27T15:44:14Z

Just discussed with @brandonh-msft regarding doc
A new PR will be submitted for it

msftclas added the cla-already-signed label Sep 22, 2016

brandonh-msft mentioned this pull request Sep 22, 2016

Deep link parsers and tests #95

Closed

ScottIsAFool reviewed Sep 22, 2016

View reviewed changes

deltakosh added this to the v1.2 milestone Sep 26, 2016

IbraheemOsama reviewed Oct 6, 2016

View reviewed changes

deltakosh added the reviewers wanted label Oct 25, 2016

deltakosh added the cannot merge due to conflicts label Oct 26, 2016

ScottIsAFool approved these changes Oct 26, 2016

View reviewed changes

Deep link parsers and tests

60773d5

nice exception for repeated keys when using DeepLinkParser more meaningful var name more single-char var name fixes xml doco fix Uri & string overloads along with UTs

brandonh-msft added 4 commits October 27, 2016 08:11

adding QueryParameterCollection to centralize the parsing & structuri…

63b475d

…ng of query parameters from a string or Uri in to a collection that can be iterated on

fixing StyleCop infractions

eb7c4fe

adding StyleCop analyzers to UT project Stylecop ignoring as did review call stack.

Update headers per build error

49bb138

file reorg

fba3107

deltakosh added need documentation 📃 and removed cannot merge due to conflicts labels Oct 27, 2016

deltakosh merged commit a21d16f into CommunityToolkit:dev Oct 27, 2016

brandonh-msft deleted the Deep-link-parsers branch October 27, 2016 16:58

Deep link parsers and tests #368

Deep link parsers and tests #368

Uh oh!

Conversation

brandonh-msft commented Sep 22, 2016

DeepLinkParser

CollectionFormingDeepLinkParser

Uh oh!

msftclas commented Sep 22, 2016

Uh oh!

ScottIsAFool Sep 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ScottIsAFool Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

ScottIsAFool Sep 22, 2016

Choose a reason for hiding this comment

Uh oh!

deltakosh commented Oct 5, 2016

Uh oh!

brandonh-msft commented Oct 5, 2016

Uh oh!

IbraheemOsama commented Oct 6, 2016

Uh oh!

IbraheemOsama Oct 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

IbraheemOsama Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

IbraheemOsama Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

IbraheemOsama Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

lukasf commented Oct 24, 2016

Uh oh!

brandonh-msft commented Oct 24, 2016

Uh oh!

deltakosh commented Oct 24, 2016

Uh oh!

brandonh-msft commented Oct 24, 2016

Uh oh!

brandonh-msft commented Oct 24, 2016

Uh oh!

lukasf commented Oct 24, 2016

Uh oh!

brandonh-msft commented Oct 24, 2016

Uh oh!

lukasf commented Oct 25, 2016

Uh oh!

ScottIsAFool commented Oct 25, 2016

Uh oh!

lukasf commented Oct 25, 2016

Uh oh!

ScottIsAFool commented Oct 25, 2016

Uh oh!

lukasf commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonh-msft commented Oct 25, 2016

Uh oh!

deltakosh commented Oct 26, 2016

Uh oh!

deltakosh commented Oct 27, 2016

Uh oh!

Uh oh!

ScottIsAFool Sep 22, 2016 •

edited

Loading

IbraheemOsama Oct 6, 2016 •

edited

Loading

lukasf commented Oct 25, 2016 •

edited

Loading