Improve error reporting

[andrewufrank]

I have just installed from github a727a02 on a debian stretch system. the version is

sprinkles --version
DumpVersion
0.6.0.0+modifications

(suggestion - it would probably more instructive if the last commit was included in the version)

I followed the 001-getting started instruction and when I run sprinkles 5000 i get

user error ("command line arguments" (line 1, column 1):
unexpected "5000")

when i used sprinkles -serve 5000 I get on opening localhost:5000 (it is not server, as one might expect!)

frank@london:~/Workspace8/testsite/sprinklesFirst$ sprinkles -serve 5000
ServeProject (ServerConfig {scBackendCache = [], scDriver = WarpDriver (Just 5000), scLogger = Nothing, scSessions = SessionConfig {sessCookieName = "ssid", sessCookieSecure = True, sessExpiration = NeverExpire, sessDriver = NoSessionDriver}, scRootDir = ""})
2019-01-01 17:34:39.470230954 UTC [Warning] Template 404.html not found, using built-in fallback
2019-01-01 17:34:39.473773224 UTC [Warning] Template 404.html not found, using built-in fallback

but nothing shows (not even 404). At least an useful error message would be appreciated when one first works with a program. Perhaps it would be better to have as a master branch a version that certainly runs and the development on identified branches and tell in the 001 instructions, which version they were tested with.

I later found the my error: the home.markdown had a mistyped extension (makrdown) which explains that nothing was found. still a helpful error message would be appreciated.

some comments: the command should have a -h and a --help text, which tells what functions are possible (I think this can be created automatically with CmdLineUtilities.UtilsProcessCmd). It seems to be customary to have -v and --version switches, but not -version.

interesting but challenging project!

[tdammers]

Thanks for the report.

Error handling / reporting is indeed a sore spot that I would like to see improved, and so is the CLI.

Some details deserve explanation:

(suggestion - it would probably more instructive if the last commit was included in the version)

I agree; however, this has a few drawbacks. First of all, if you build an uncommitted source tree, you would still have to append some generic tag like +modifications to the version, so it'd still be a bit of a mess. Then, adding the actual commit hash to the version number introduces an implicit build dependency on git, or, more specifically, on the build happening on a git working copy, which isn't necessarily the case (e.g. if you build from a downloaded tarball). We still have this dependency, but we can gracefully and silently recover if it isn't met - essentially, if we're not in a git repo, then we'll assume that whatever version number the Cabal file gives us is exactly what we're building. Further, in a realistic scenario, a deployed binary on a production system is generally going to be a "proper" version (i.e., some properly versioned release without any additional modifications), so there is very little added value in going through the hassle of including the actual commit hash in the reported version number.

Actually, after checking the sources, I remembered that we are already doing exactly what you're asking:

• If you build from a non-git source tree, then the reported version is simply whatever the Cabal file says (e.g. 0.6.0.0).
• If you build from a clean git source tree at a tagged commit, and the tag agrees with the Cabal file, the version is the tag name (e.g. 0.6.0.0)
• If the tag doesn't agree with Cabal, the Cabal version number is leading, but the tag is appended between parentheses (e.g. 0.6.0.0 (implement-foo-feature))
• If you build from a clean git source tree at an untagged commit, the version is the version number from the Cabal file plus +git- plus the git commit hash (e.g. 0.6.0.0+git-1a5afdf35dd247ac14e5d2f02a4b11c07a3c8989)
• If you build from an unclean working copy (which is defined as a working copy for which git status --porcelain produces nonempty output), +modifications is appended to the version string.

This means that your 0.6.0.0+modifications build is based on a git working copy at tag 0.6.0.0, with either local modifications or non-ignored untracked files in the working copy.

I followed the 001-getting started instruction and when I run sprinkles 5000 i get

Ah, terrible shame - the CLI syntax got changed at some point. Previously, sprinkles could only really do one thing, namely, serving the current project, and you would pass options to control the details (which HTTP backend to use, which port to run on, etc.). But with the introduction of the -bake command, this got changed, and so now if you want sprinkles to actually serve the project, you have to explicitly pass -serve. The whole CLI options could use an overhaul, but I'm slightly reluctant to introduce breaking changes here. In any case, I updated the relevant line in the Getting Started guide.

it is not server, as one might expect!

It's a verb, just like "bake". sprinkles -serve 5000 means "Sprinkles, please serve this project on port 5000", just like sprinkles -bake ./baked means "Sprinkles, please bake this project and store the result in the baked subdirectory".

but nothing shows (not even 404). At least an useful error message would be appreciated when one first works with a program.

Sprinkles should very much produce a bare-bones 404 page, saying something like "Not found" in plain text, when the given path doesn't resolve to any rule, or when the matching rule cannot load a required data item. Likewise, if there is some sort of error while handling the request, like a syntax error in a template, or a crash in a data backend, Sprinkles should serve a default plain-text 500 page. Exposing any more error information than that to the browser is generally considered a security problem (and also a bad thing to do for other reasons), and is best avoided entirely. I do agree however that the error log should contain more useful information, and that it would be helpful to configure more verbose error logging in the tutorial.

Perhaps it would be better to have as a master branch a version that certainly runs and the development on identified branches and tell in the 001 instructions, which version they were tested with.

The tutorial is part of the git source tree, and the intention is that it matches the version of whatever commit you are looking at. I must admit however that I do not check the tutorial for every commit, or even every release, so it is very possible to be incorrect. Sorry about that.

I later found the my error: the home.markdown had a mistyped extension (makrdown) which explains that nothing was found. still a helpful error message would be appreciated.

Agree; logging which exact file sprinkles failed to find might be convenient. There is one caveat with this, which is that it isn't always obvious whether a file is missing by accident or on purpose. Sometimes, you want to load a file if it exists, and silently get null in its place if it doesn't. If the file is in the required list for that rule, then it is obvious that silent null isn't desirable, but otherwise, sprinkles can't tell if the file should have been there.

the command should have a -h and a --help text, which tells what functions are possible (I think this can be created automatically with CmdLineUtilities.UtilsProcessCmd).

Yes, --help should absolutely be supported. The main reason it isn't is negligence on my part. Due to the way the argument parser works, it cannot be created automatically, but that's a lousy excuse. An overhaul should either add help text manually, or switch to something like argparse-applicative, which gives you --help for (almost) free.

It seems to be customary to have -v and --version switches, but not -version.

Again, a bit of negligence here. In principle, the convention for sprinkles args is to have GNU-style single-dash keyword args, but for mere convenience, and to avoid confusion, we should support -h, --help, -v, and --version as well. In fact, I'm willing to reconsider the choice entirely, the only valid argument against would be backwards compatibility.

EDIT:

Actually, I just checked; --version and -v should also just work. If they don't, then please do file a new bug report specifically.