hugolib: Allow arrays of arrays in frontmatter Params #2841
Conversation
| // Page Params | ||
| pages := sites.findAllPagesByKind(KindPage) | ||
| for _, p := range pages { | ||
| if p.Path() == "sect1/regular1.en.md" { |
There was a problem hiding this comment.
Are you sure the above is truthful on Windows?
There was a problem hiding this comment.
Also, when you need to add an import to get to the assert package, you by definition is breaking the standard set in the file. I think the require package is suitable in most situations. If something fails, it is bad and I want it to stop.
There was a problem hiding this comment.
Also, and I know I have been a sinner in area, adding more test cases to an existing very broad test to test for a very specific thing smells a little. I think we have narrower unit-typish tests for these params conversions -- and if not, we should.
There was a problem hiding this comment.
-
Good catch. I need to use
filepath.Join. -
I didn't notice your standard. Apparently, I have a different standard. 😁 I use
assertwhere I would uset.Errorfandrequirewhere I would uset.Fatalf. I only useFatalf/requireto avoid accessing nil pointers and such in subsequent tests. So, when I userequire, it's a signal to me that we've got a serious problem. If a Test func contains numerous assertions, I like to see them all fail instead of just the first one. WithrequireI'd only see the first failured assertion. If you don't like my reasoning, I'll change torequirein this file. -
I agree, but I was trying to follow your lead. 😀 Sometimes I'm not quite sure how to setup all the site scaffolding to run a simple unit test. I'll take another look at the tests in this PR to see if I can pull them out into a more narrow test.
There was a problem hiding this comment.
The node_as_page_test was something I built to be able to move from node to page, and is a little bit of everything, and I was thinking of maybe remove it, or move the tests.
Testing TOML to interface{} conversions sounds like a job for a unit test.
As to require: If you have one failing test, you have a serious problem by (my) definition -- and fixing that one test is usually enough to fix any tests that come after it. But up to you, my point was: If there are 100 require in a test file, then adding one single assert is at least breaking a pattern.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fixes #2752