Refactor #120
Refactor #120
Conversation
There was a problem hiding this comment.
This is great @padraicbc, thanks for digging through all this. It helps me recognize some bad patterns I've gotten used to and gets us closer on feedback I've received about better error handling!
| @@ -18,6 +18,7 @@ func Bundle() []byte { | |||
| //Externals: []string{"module", "fs", "path"}, | |||
| Bundle: true, | |||
| }) | |||
| // error? | |||
There was a problem hiding this comment.
yeah that probably would be a good idea, bundle.go isn't actually being used at the moment... I had left it in there thinking it could be useful for pulling in libs or bundling for production, but we could probably remove it entirely for now.
|
|
||
| // Get svelte compiler code from node_modules. | ||
| compiler, err := ioutil.ReadFile(tempBuildDir + "node_modules/svelte/compiler.js") | ||
| compiler, err := ioutil.ReadFile( | ||
| fmt.Sprintf("%snode_modules/svelte/compiler.js", tempBuildDir), |
There was a problem hiding this comment.
I noticed these in your last PR, curious what advantage fmt.Sprintf() has over concatenating with +? I figured it was performance, but doesn't seem the case: https://dev.to/pmalhaire/concatenate-strings-in-golang-a-quick-benchmark-4ahh
There was a problem hiding this comment.
+ would be faster bar you were maybe using Fprintf' with a responseWritervs callingw.Write(a+b+c)`. It is just personal preference/habit and probably did it on autopilot. I use it more so when you have a few substrings as I find it easier to see if fat finger syndome has occurred.
| typeContentPath := fmt.Sprintf("content/%s", strings.Trim(typeName, " /")) | ||
| // !os.IsNotExist is true, the path exists. os.IsExist(err) == nil for Stat if file exists | ||
| if _, err := os.Stat(typeContentPath); !os.IsNotExist(err) { | ||
| fmt.Printf("A Type content source with the same name located at \"%s/\" already exists\n", typeContentPath) | ||
| return | ||
| // an error? |
There was a problem hiding this comment.
Sure, we could just return the Printf above as an error.
| @@ -51,11 +52,12 @@ Optionally add a _blueprint.json file to define the default field structure for | |||
| Run: func(cmd *cobra.Command, args []string) { | |||
| typeName := args[0] | |||
|
|
|||
| // shoud we stop here on error from either? | |||
There was a problem hiding this comment.
Hmm, that's a good question... should you be able to complete either the data source or template if the other exists. There currently is the concept of a layout/content/ template without a content source (like 404.svelte). There are also types in the content/ folder without templates (No endpoint content: #13 (comment)). Yeah it might be best it if at least warns you about this. Currently it looks like this in the CLI:
plenti new type test
Creating new Type content source: content/test/
Creating new Type layout: layout/content/test.svelte
plenti new type test
A Type content source with the same name located at "content/test/" already exists
A Type layout with the same name located at "layout/content/test.svelte" already exists
rm -R content/test
plenti new type test
Creating new Type content source: content/test/
A Type layout with the same name located at "layout/content/test.svelte" already exists
There was a problem hiding this comment.
So there can be three scenarios:
- A content/data source with a corresponding template.
- A content/data source without corresponding template.
- And a template without a data source.
Is there any situation where adding a template or data source breaks anything, say foo.svelte exists without content/foo(or vice versa) and then you plenti new type foo?
There was a problem hiding this comment.
Yup, so it should work in all cases:
- content with template = regular node endpoint(s) you can visit
- content without template = content you can pull into other nodes, but has no direct node endpoint itself
- template without content = rare, but currently used to handle 404 errors. You could eject the router to handle more cases.
Completing the set by either adding a template or data source shouldn't break anything, but might surprise the user with unintended consequences. For instance let's say the user inherited a site that put the main menu links into a content type called links. This existed as a content source without a template so there were no endpoints for the actual links (they may have pointed to nodes created by a different content type). If the user then tries to create a new content type called links for a different purpose, not realizing it already existed, the effect is a template will be created and it will expose all the existing links in the content source as node endpoints. In reality this would probably be discovered pretty quickly and with git the rollback would be trivial so I'm not super worried about this usecase - just wanted to put it out there.
| typeContentPath := fmt.Sprintf("content/%s", strings.Trim(typeName, " /")) | ||
| // !os.IsNotExist is true, the path exists. os.IsExist(err) == nil for Stat if file exists | ||
| if _, err := os.Stat(typeContentPath); !os.IsNotExist(err) { | ||
| fmt.Printf("A Type content source with the same name located at \"%s/\" already exists\n", typeContentPath) | ||
| return | ||
| // an error? | ||
| return nil | ||
|
|
||
| } | ||
|
|
||
| if _, err := os.Stat(typeContentPath + ".json"); !os.IsNotExist(err) { | ||
| // error or not? |
There was a problem hiding this comment.
We could error this too per the notes above. Best UX I think would be adding promptui to confirm, but that can happen down the road. Either way these commands are little helpers that you could easily get around manually if you were unhappy with us cutting out early.
No worries. I think 95% of errors should be handled. Again I did a quick sweep thorugh the code so there are probably parts I have exited that you may want to give options or continue but it should be all in the build steps outside of the functions themselves so easy to test them and change the flow if needed. |
I did end up killing the check in fsnotify.Create (d597450) because it stopped the local server on Linux, I think because of the way it performs temp file writing. Also saw your note (thank you) about |
Again some refactoring and handling unchecked errors. Wrapping errors ( wrapped with %w which may or may not suit. ) where appropriate and returning. Keeping any handling(log.Fatal...) outside to be able to easily test.
Addded common package with one simple func there to check errors. Can add more logic like the wrapping of errors etc.. to keep in one location.
Few other general changes like the fsnotify.Watcher logic which quits on inabilty to create/setup. Not the prettiest but works...