initial logic #34

s · 2017-06-07T16:02:08Z

In this PR, I have implemented the initial architecture and logic for CoffeeBot where we have several stubs for several methods&services.
CoffeeBot implements Bot protocol and has one main class to handle requests which is CofeeManager. Other than that there are many helper services which accomplish individual tasks such as EntityRecognitionService or SessionService etc.

makadaw · 2017-06-07T16:10:19Z

Sources/Core/CoffeeManager.swift

+
+import Foundation
+
+class CoffeeManager : NSObject {


Why we need NSObject here?

I think for now we don't need it. I am removing inheritance from NSObject.

atkit · 2017-06-28T06:09:41Z

feels like something wrong with merge, why does it contain so many old commits ?

atkit · 2017-06-28T06:10:53Z

I see, it feels like it needs rebase + force push (don't worry, it is handled by github in positive way)

If you have question how to do that, please feel free to come to us today

This commit adds the initial stub for AppointingService which chooses a person to prepare the coffees from given session

This commit adds initial stub for EntityRecognitionService which parses given string to entities such as Coffee

This commit adds initial stub for SessionService which has functionalities to store and restore the session

This commit adds initial stub for SpellCheckingService which normalizes a given text

Add inital stub for TextProcessingService which processes the given text and return a result

This commit updates the description of Coffee model where now it returns all descriptions of additions as string

This commit updates coffee property to become an array instead of one Coffee since a person may request multiple coffees

This commit deletes old spell checker file since we can always restore it from .git history to use in new SpellChecker class.

makadaw

Add few questions and comments

makadaw · 2017-06-29T05:54:20Z

Sources/CoffeeBot/CoffeeManager/CoffeeManager.swift

+    func process(activity:Activity)
+}
+
+class CoffeeManager {


What is the purpose of this entity?

I thought of CofeeBot as a main.m which speaks with the external modules and this CoffeeManager handles all the internal services and everything.

Ok, I think you can merge this class to CoffeeBot. And then extend CoffeeBot with Bot protocol.
So you have entry point to your Bot and you can test it as you want and in same time extending from Bot protocol give you ability to integrate it inside BotsKit

makadaw · 2017-06-29T05:57:29Z

Sources/CoffeeBot/CoffeeBot.swift

+    }
+
+    //MARK: Lifecycle
+    public init() {


You can set dependencies of this class (SessionService, Appointng, etc) as parameters for init and provide default implementations as default value
public init(sessionService: SessionService = SessionService(), ...)
With this approach it's easy to write unit tests and inject dependecies.

I think you are right I will create a factory to create CoffeeBot, in this way we will be able to inject dependencies.
I would like to create own factory class because I think external classes don't necessarily know about all internal types such as EntityRecognitionService.

makadaw · 2017-06-29T05:59:14Z

Sources/CoffeeBot/CoffeeManager/CoffeeManager.swift

+    weak var botDelegate : CoffeeBot?
+
+    //MARK: Lifecycle
+    init(textProcessingService:TextProcessingService, appointingService:AppointingService, sessionService:SessionService) {


What do you think about close this Services by protocols?

That is a really good point, I will think of this. I only have one concern which is:
If I receive something like init(services:[CoffeeBotServiceProtocol]) since all services have different functionality and method signatures I am not sure if I can unify them.

I mean create protocols for each service and then create implementation for each protocol, this can help you to tests your bot

makadaw · 2017-06-29T06:00:18Z

Sources/CoffeeBot/CoffeeManager/CoffeeManager.swift

+        return currentSession.membersReplied.count == conversation.members.count
+    }
+
+    fileprivate func isTimeoutPeriodFinished() -> Bool {


Good point I think we need to add some timer pulling into BotsKit

makadaw · 2017-06-29T06:01:46Z

Sources/CoffeeBot/CoffeeManager/CoffeeManager.swift

+class CoffeeManager {
+
+    //MARK: Properties
+    fileprivate var session : Session? = nil


What if the bot will process few session in the same time?

I think there could be only one session at a time. Because if a second session is started before the first one is ended how will we differentiate between which coffee request came to which session?

This is a good question :) Here is the difference between mobile app and backend, one app needs to process different requests at the same time.
Let's discuss this on next meeting!

makadaw · 2017-06-29T06:03:27Z

Sources/CoffeeBot/CoffeeManager/CoffeeManager.swift

+            self.sessionService.save(session: session)
+
+        case .error(let error):
+            Log.error("An error occured during processing message:\(error.localizedDescription)")


I think good variant will ask user repeat his message

makadaw · 2017-06-29T06:05:26Z

Sources/CoffeeBot/Models/Coffee.swift

+
+internal struct Coffee {
+    let amount : Int
+    let type : CoffeeType


What do you think about internal type in Coffee namespace? Something like:
let type: Coffee.Type

Hmm, I never have done this before. Can you share an example?

Sure, https://github.com/s/SwiftBot/blob/master/Sources/ChatProviders/Facebook/MessengerWebhook.swift#L72
or in your case:

internal struct Coffee { let amount : Int enum `Type`: String { case espresso case capuchino } let type: Type }

Then you can extend your type with this:

extension Coffee.Type { func ... }

s requested review from BogdanSr, atkit, goudsmit and makadaw June 7, 2017 16:02

makadaw approved these changes Jun 7, 2017

View reviewed changes

Said Ozcan added 6 commits June 27, 2017 16:29

initial logic

2149245

remove inheritance from NSObject

2b5c3b2

add dependencies to Package.swift for CoffeeBot

dab2370

delete unnecessary files

1b3db3a

add models needed for CoffeeBot

69c27c1

add initial impl. of CoffeeBot and CoffeeManager

1cdea8f

Said Ozcan added 17 commits June 28, 2017 15:31

initial logic

d6b3971

add AppointingService

955b33f

This commit adds the initial stub for AppointingService which chooses a person to prepare the coffees from given session

add EntityRecognitionService

f730f35

This commit adds initial stub for EntityRecognitionService which parses given string to entities such as Coffee

add SessionService

cb3c88c

This commit adds initial stub for SessionService which has functionalities to store and restore the session

add SpellCheckingService

6623c4f

This commit adds initial stub for SpellCheckingService which normalizes a given text

add TextProcessingService

f10ff07

Add inital stub for TextProcessingService which processes the given text and return a result

add amount property to Coffee

dadd20b

update description of Cofee model

83d96bc

This commit updates the description of Coffee model where now it returns all descriptions of additions as string

move CoffeeManagerInputProtocol to CoffeeManager

7313101

move CoffeeBotInputProtocol to CoffeeBot

c9ca73f

update CoffeeRequest:cofee property

dc74826

This commit updates coffee property to become an array instead of one Coffee since a person may request multiple coffees

add description to CoffeeRequest model

bbb0221

make coffeesRequested property mutable in Session

a8bc408

update description property in Session

4661e5a

delete old files again

777e527

initializing CoffeeManager with all services

79da0f4

implement initial flow for processing a message

20f5f5e

delete old spellchecker file

4686a88

This commit deletes old spell checker file since we can always restore it from .git history to use in new SpellChecker class.

s force-pushed the initial_logic branch from 334e5f3 to 4686a88 Compare June 28, 2017 13:32

makadaw reviewed Jun 29, 2017

View reviewed changes

initial logic #34

Are you sure you want to change the base?

initial logic #34

Uh oh!

Conversation

s commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atkit commented Jun 28, 2017

Uh oh!

atkit commented Jun 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

makadaw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

s commented Jun 7, 2017 •

edited

Loading

atkit commented Jun 28, 2017 •

edited

Loading