Closed rmasci closed 2 years ago
I am so sorry. I thought i pushed those to The fork in my github. That code is not ready just testing out the code then I was going to go back and clean it up.
Thank you for the code review by the way. Will pushing to my github send you a pull request?
Thanks.
Rich
On Fri, May 29, 2020 at 7:53 PM Noah Petherbridge notifications@github.com wrote:
@kirsle requested changes on this pull request.
Thanks for the pull request!
In rivescript.go https://github.com/aichaos/rivescript-go/pull/40#discussion_r432782921:
- if cfg.Persist == true && cfg.DBfile != "" {
- var err error
- cfg.SessionManager, err = skv.New(cfg.DBfile)
- if err != nil {
- fmt.Printf("Error Opening Database Try turning persist off.\n", err)
- os.Exit(1)
- }
- } else {
- cfg.SessionManager = memory.New()
- }
I'd prefer that third-party session managers not be referenced directly in the core of rivescript-go. Users who want to use the default in-memory store or the Redis one (for example) shouldn't need to install the Go dependencies for rapidloop/skv if they don't want to use this one.
Usage of this session store should instead be up to the author of the bot program:
package main import ( "github.com/rapidloop/skv" "github.com/aichaos/rivescript-go" ) func main() { skv, _ := skv.New("my.db") bot := rivescript.New(&rivescript.Config{ SessionManager: skv, }) }
See the redis driver https://github.com/aichaos/rivescript-go/tree/master/sessions/redis for example.
In sessions/redis/redis.go https://github.com/aichaos/rivescript-go/pull/40#discussion_r432783027:
@@ -1,4 +1,4 @@ -// Package redis implements a Redis backed session manager for RiveScript. +a// Package redis implements a Redis backed session manager for RiveScript.
This looks like a typo that should be removed.
In sessions/skv/skv.go https://github.com/aichaos/rivescript-go/pull/40#discussion_r432783612:
@@ -0,0 +1,266 @@ +// Package memory provides the default in-memory session store with skv backing the memory stuff up
This file looks good apart from the documentation ("// Package memory ..."), new session driver options are always welcome!
In loading.go https://github.com/aichaos/rivescript-go/pull/40#discussion_r432785164:
@@ -88,6 +118,113 @@ func (rs *RiveScript) LoadDirectory(path string, extensions ...string) error { return nil }
+/* +LoadHttpFilesystem loads multiple RiveScript documents from a folder on disk. + +Parameters +
- path: Path to the directory on disk
- extensions...: List of file extensions to filter on, default is
- '.rive' and '.rs' +/ +func (rs RiveScript) LoadHttpFilesystem(httpfs http.FileSystem, path string, extensions ...string) error {
- if len(extensions) == 0 {
- extensions = []string{".rive", ".rs"}
- }
- files, err := vfspath.Glob(httpfs, fmt.Sprintf("%s/*", path))
I'd rather not have this dependency be part of the rivescript-go core (see RiveScript Goals and Scope https://www.rivescript.com/contributing). Currently the core of rivescript-go contains no third-party dependency outside the Go standard lib, and modules that do bring dependencies (sessions/redis, lang/javascript) must be chosen by the program author when they use RiveScript in their code.
LoadHttpFile() is okay to stay since it uses the standard net/http package.
It looks like you want this function to help with the bot program you're making, but this is probably better left as an implementation detail to your program and kept out of the rivescript-go core.
In loading.go https://github.com/aichaos/rivescript-go/pull/40#discussion_r432785389:
- return fmt.Errorf("No RiveScript source files were found in %s", path)
- }
- return nil +}
+/* +LoadMultipleDirectory loads multiple RiveScript documents from a comma separated list of folders on disk.
+Parameters
- comma separated list of paths: Paths to the directorys on disk
- extensions...: List of file extensions to filter on, default is
- '.rive' and '.rs' +/ +func (rs RiveScript) LoadMultipleDirectory(paths []string, extensions ...string) error {
The documentation on this function is inaccurate since paths is a string slice and not a comma separated string.
And maybe a more semantic name for this function would be LoadDirectories()?
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/aichaos/rivescript-go/pull/40#pullrequestreview-421349482, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGRIPKPCLWFFYV3LBJZDOLRUBDHVANCNFSM4HGWBXFA .
Added a function called LoadMultipleDirectories to loading.go. This takes a []string instead of a string, so that the user can supply more than one directory.
I need this because for the users of my chatbot I have a stock set of rivescript files that give basic "ALICE" like responses, but there are a bunch of rivescript files that they never need to edit, so move these to a default directory. The user can then put any of their customizations in their own directory that is specified on startup. In my chatbot I load the customized location after the default so that it overwrites any directives in the default rivescript folder.