Search for metadata files in $DATADIR/metadata #7851
Conversation
3c25cd7 to
33048a8
Compare
src/Text/Pandoc/App.hs
Outdated
| paths -> mconcat <$> | ||
| mapM (\path -> do raw <- readFileStrict path | ||
| yamlToMeta readerOpts (Just path) raw) paths | ||
| paths -> do | ||
| mconcat <$> | ||
| mapM | ||
| (\path -> do | ||
| raw <- readMetadataFile path | ||
| yamlToMeta readerOpts (Just path) raw) | ||
| paths |
There was a problem hiding this comment.
Here it would be helpful if your change were the minimal one necessary: just changing readFileStrict to readMetadataFile and leaving everything else the same. With the reformatting, it becomes more difficult to see what has changed. (Also, there's now a redundant do.)
There was a problem hiding this comment.
Absolutely, sorry about that. Changed in fixup.
src/Text/Pandoc/Class/PandocMonad.hs
Outdated
| if existsInWorkingDir then readFileStrict fname | ||
| else do | ||
| dataDir <- getUserDataDir |
There was a problem hiding this comment.
The style of the rest of the pandoc code is:
if foo
then bar
else baz
Better to keep to a consistent style.
There was a problem hiding this comment.
Changed in fixup.
src/Text/Pandoc/Class/PandocMonad.hs
Outdated
| let path = userDir </> "metadata" </> fname | ||
| existsInUserDir <- fileExists path | ||
| if existsInUserDir then readFileStrict path | ||
| else throwError $ PandocCouldNotFindDataFileError $ T.pack fname |
There was a problem hiding this comment.
This isn't really the appropriate error since it's not a data file.
Maybe we need a new constructor on PandocError.
There was a problem hiding this comment.
I could add PandocCouldNotFindMetadataFileError? That would be consistent with readDataFile and readMetadataFile.
There was a problem hiding this comment.
Added PandocCouldNotFindMetadataFileError.
There was a problem hiding this comment.
Note that a missing metadata file specified with --metadata-dir results in the more general PandocIOError (via readFileStrict) in master. This PR changes that.
| @@ -0,0 +1,18 @@ | |||
| ``` | |||
There was a problem hiding this comment.
Usually we just name these with the number, e.g. 5876.md.
There was a problem hiding this comment.
I'll try to write the other tests that you proposed too. There appears to be a convention for enumerating tests that belong to the same issue, e.g. 5876a.md, 5876b.md. Is that OK?
Should mention that CONTRIBUTING.md suggests using both issue number and feature in the test file names.
There was a problem hiding this comment.
Changed in fixup.
There was a problem hiding this comment.
Oh, didn't realize that about contributing.md!
I guess it's probably not bad advice, and it looks like we've done it sometimes.
So, as you like.
For multiple tests belonging to the same issue, simply include them all in the same file. A command test file can contain any number of command tests (separate code blocks). We don't really have a convention of putting them in separate files with letter suffixes.
There was a problem hiding this comment.
Ah, I see. I gathered all tests in 5876.md then. I think an extra suffix is unnecessary in this case since the tests are pretty clear about what is being tested.
| @@ -0,0 +1,4 @@ | |||
| --- | |||
There was a problem hiding this comment.
You'll need to make sure this test file is included in extra-source-files in pandoc.cabal, or it won't be included in the source distribution.
Maybe also worth testing two further cases:
- the case where the file exists in the working directory AND in DATADIR/metadata
- the case where the file exists in neither place (yielding an error)
There was a problem hiding this comment.
Added resources to extra-source-files as well as two new tests. There's also the case where the file exists in the working directory, but not in the user directory, which is covered by multiple existing tests. I could still add it under this issue for completeness, though.
There was a problem hiding this comment.
Note that you shouldn't put 7851.md and the like in extra-source-files, because it's already covered by a glob (test/command/*.md). However, the yaml file in the subdirectory wasn't covered by a glob. Note that make check-cabal will automatically check to make sure that there aren't committed test files that aren't in the extra-source-files stanza. I don't think we necessarily need the extra test case you describe, but it wouldn't hurt. Up to you.
There was a problem hiding this comment.
Great, I ran the check and everything should be included now. I also added a fourth test for when there's no user directory specified and the file does not exist. Then all four possible outcomes of readMetadataFile are covered in 5876.md.
3dcd6f5 to
312b1e5
Compare
|
Squashed the fixups into a single commit. |
|
This looks great! Just a couple small things now.
|
If files specified with --metadata-file are not found in the working directory, look in $DATADIR/metadata. Expose new readMetadataFile function from T.P.Class (PandocMonad) [API change]. Expose new PandocCouldNotFindMetadataFileError constructor from T.P.Error (PandocError) [API change]. Closes jgm#5876.
312b1e5 to
397153d
Compare
Like this? |
|
Now that I've merged this, a potential issue occurs to me. |
|
Hm, can't imagine that anyone wants to do that on purpose. I checked the behaviour for |
|
One approach would be to allow subdirectories in both cases, but only if they don't contain |
|
Created #7861. |
If files specified with --metadata-file are not found in the working
directory, look in $DATADIR/metadata.
Closes #5876.