Skip to content
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

Rewrite skill as .NET Core #124

merged 47 commits into from Oct 22, 2019

Rewrite skill as .NET Core #124

merged 47 commits into from Oct 22, 2019


Copy link

@martincostello martincostello commented Oct 20, 2019

This PR rewrites the node.js and JavaScript code for the skill Lambda into a .NET Core skill instead.

Add the skeleton solution files and edits to migrate the skill to .NET Core from node.js.
Split into module name and handler name (might be wrong).
Add zip argument to deploy the publish output.
Remove trailing comma.
Add a new temporary function for testing deployment of the .NET Core version of the skill.
Fix the format of the handler name as it doesn't get constructed correctly (uses . instead of ::).
Update the IAM policy template to remove redundant role and add missing role.
Try and work around the default module name being wrong but the Travis CI tool not using the correct separator.
Use LogLine() instead of Log().
Update the basic response to set the version and response body.
Remove the node.js version of the skill to de-clutter ahead of the re-implementation.
Rename the function entry-point class.
Update class documentation.
Configure for .NET code rather than JavaScript.
Add skeleton implementation of the London Travel skill.
Update the assets for Visual Studio Code for .NET Core.
Implement the simple Cancel/Stop, Help, Launch and Stop handlers and add tests to ensure match existing output.
Fix bugs in implementation found by those tests plus some general refactoring.
Remove unused local variable.
Setup dependency injection to prepare for implementing the intents that need to call the TfL API or skill API.
Fix incorrect intent name.
Add missing dependency injection registration for the JsonSerializerOptions class.
Add infrastructure to support intercepting HTTP requests to external services using bundle files.
Removed unused serialization code from SystemTextJsonContentSerializer.
Implement the intent for getting any disruption.
Increase the amount of memory allowed so that the DI container has enough memory to initialize.
Add a test for if getting disruption fails for some reason.
Implement the status intent.
Refactor building SSML responses.
Remove some redundant code branches.
Change default HTTP timeout to 5 seconds.
Log slot values in telemetry.
Use a class similar to HttpContextAccessor to flow the Lambda execution around to requests for access via dependency injection.
Use a resource file for most of the resource strings.
Add some more resource strings for the status intent.
Guard against invalid locale values when setting the culture.
Add the prescence of an access token (or not) to the telemetry properties.
Exclude unused method from code coverage.
Implement the correct response shape etc. for ISkillClient and add the parameter to set the access token.
Add support for returning a link account card.
Update the different bundles to not have conflicting Ids.
Implement the commute intent and add tests for main flows.
Log the locale of the current request.
Set CultureInfo.CurrentCulture instead of Thread.CurrentThread.CurrentCulture.
Nest the en-GB resx under Strings.resx.
Change the neutral resources language from en to en-US.
It isn't working on Linux and I don't know why, so just acquiesce and make the default UK English.
Add test cases for remaining uncovered code.
Remove code that testing proved to be redundant.
Remove build and deploy for branch now that it's done.
@martincostello martincostello added enhancement dependencies labels Oct 20, 2019
@martincostello martincostello added this to the Future milestone Oct 20, 2019
@martincostello martincostello self-assigned this Oct 20, 2019
Copy link

codecov bot commented Oct 20, 2019

Codecov Report

Merging #124 into master will decrease coverage by 1.31%.
The diff coverage is 98.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #124      +/-   ##
- Coverage     100%   98.68%   -1.32%     
  Files          16       24       +8     
  Lines         466      532      +66     
  Branches       91        0      -91     
+ Hits          466      525      +59     
- Misses          0        7       +7
Flag Coverage Δ
#linux 98.68% <98.68%> (?)
Impacted Files Coverage Δ
src/LondonTravel.Skill/IntentFactory.cs 100% <100%> (ø)
src/LondonTravel.Skill/Intents/CommuteIntent.cs 100% <100%> (ø)
src/LondonTravel.Skill/Intents/DisruptionIntent.cs 100% <100%> (ø)
...rc/LondonTravel.Skill/Clients/ServiceDisruption.cs 100% <100%> (ø)
src/LondonTravel.Skill/Clients/Line.cs 100% <100%> (ø)
src/LondonTravel.Skill/LambdaContextAccessor.cs 100% <100%> (ø)
src/LondonTravel.Skill/SkillResponseBuilder.cs 100% <100%> (ø)
...LondonTravel.Skill/Clients/SkillUserPreferences.cs 100% <100%> (ø)
src/LondonTravel.Skill/Lines.cs 100% <100%> (ø)
src/LondonTravel.Skill/Clients/LineStatus.cs 100% <100%> (ø)
... and 38 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b0fac64...119ddaf. Read the comment docs.

Some code refactoring to reduce the size of methods as recommended by Code Climate.
Remove node.js instructions.
Copy link
Owner Author

martincostello commented Oct 22, 2019

Things to think about for the future:

  • What's best practice for logging?
  • What's best practice for DI?

Remove redundant exclusions.
Update Moq and ReportGenerator to their latest versions.
@martincostello martincostello merged commit c545fee into master Oct 22, 2019
3 of 6 checks passed
@martincostello martincostello deleted the DotNet-Core branch Oct 22, 2019
@martincostello martincostello modified the milestones: Future, 2.0.0 Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
dependencies enhancement
None yet

Successfully merging this pull request may close these issues.

None yet

1 participant