diff --git a/CHANGELOG.md b/CHANGELOG.md --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,15 @@ # `nbparts` Changelog +## v0.1.1.0 + +- Fix incorrect serialisation of `FormatJson` in the manifest file. + + - For backward compatibility, `nbparts` will still be able to deserialise the incorrect representation. + +- Media directories (`media` and `outputs-media`) will now only be created if necessary (i.e., when there are media to export). + +- Packing will no longer fail when the outputs file is missing. Instead, `nbparts` will warn and assume that the notebook has no outputs. + ## v0.1.0.0 - Initial release. diff --git a/README.md b/README.md --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Build](https://github.com/dixslyf/nbparts/actions/workflows/build.yaml/badge.svg?branch=master&event=push)](https://github.com/dixslyf/nbparts/actions/workflows/build.yaml) [![Build (Nix)](https://github.com/dixslyf/nbparts/actions/workflows/build-nix.yaml/badge.svg?branch=master&event=push)](https://github.com/dixslyf/nbparts/actions/workflows/build-nix.yaml) +![Hackage Version](https://img.shields.io/hackage/v/nbparts?label=Hackage) `nbparts` is a tool for splitting Jupyter notebooks into its "parts": @@ -83,10 +84,6 @@ cabal install nbparts ``` -With the default Cabal configuration, -this will install `nbparts` into `~/.cabal/bin` on Linux / macOS -and `%APPDATA%\cabal\bin` on Windows. - ### Pre-Built Binaries Static binaries for x86_64 Linux are available from the [releases](https://github.com/dixslyf/nbparts/releases). @@ -140,10 +137,6 @@ ``` cabal install ``` - -With the default Cabal configuration, -this will build and install `nbparts` into `~/.cabal/bin` on Linux / macOS -and `%APPDATA%\cabal\bin` on Windows. ### Nix diff --git a/nbparts.cabal b/nbparts.cabal --- a/nbparts.cabal +++ b/nbparts.cabal @@ -1,6 +1,6 @@ cabal-version: 3.8 name: nbparts -version: 0.1.0.0 +version: 0.1.1.0 synopsis: Unpack a Jupyter notebook into its sources, metadata and outputs description: `nbparts` is a tool for splitting Jupyter notebooks into its "parts": diff --git a/src/Nbparts/Pack.hs b/src/Nbparts/Pack.hs --- a/src/Nbparts/Pack.hs +++ b/src/Nbparts/Pack.hs @@ -100,8 +100,7 @@ Monad.unless cont $ liftIO (Text.hPutStrLn stderr "Operation cancelled: file not overwritten") Monad.guard cont - -- Read metadata, sources and outputs - -- TODO: Don't fail if metadata and outputs are missing — just warn. + -- Read sources. let sourcesPath = mkImportPath "sources" sourcesFormat (sources :: [CellSource]) <- case sourcesFormat of FormatYaml -> do @@ -114,6 +113,7 @@ mdText <- liftIO $ Text.readFile sourcesPath liftEither $ markdownToSources sourcesPath mdText + -- Read metadata. let metadataPath = mkImportPath "metadata" metadataFormat (metadata :: NotebookMetadata) <- case metadataFormat of FormatYaml -> do @@ -124,15 +124,22 @@ liftEither $ left (PackParseJsonMetadataError . Text.pack) res _ -> throwError $ PackIllegalFormatError IllegalFormatMetadata metadataFormat + -- Read outputs. let outputsPath = mkImportPath "outputs" outputsFormat - (unembeddedOutputs :: UnembeddedNotebookOutputs) <- case outputsFormat of - FormatYaml -> do - res <- liftIO $ Yaml.decodeFileEither outputsPath - liftEither $ left (PackParseYamlOutputsError . ParseYamlError) res - FormatJson -> do - res <- liftIO $ Aeson.eitherDecodeFileStrict outputsPath - liftEither $ left (PackParseJsonOutputsError . Text.pack) res - _ -> throwError $ PackIllegalFormatError IllegalFormatOutputs outputsFormat + outputsPathExists <- liftIO $ Directory.doesFileExist outputsPath + (unembeddedOutputs :: UnembeddedNotebookOutputs) <- + if outputsPathExists + then case outputsFormat of + FormatYaml -> do + res <- liftIO $ Yaml.decodeFileEither outputsPath + liftEither $ left (PackParseYamlOutputsError . ParseYamlError) res + FormatJson -> do + res <- liftIO $ Aeson.eitherDecodeFileStrict outputsPath + liftEither $ left (PackParseJsonOutputsError . Text.pack) res + _ -> throwError $ PackIllegalFormatError IllegalFormatOutputs outputsFormat + else do + liftIO $ Text.hPutStrLn stderr "Warning: Could not find outputs file — assuming no outputs" + pure mempty let (NotebookMetadata major minor _ _) = metadata nb <- case major of diff --git a/src/Nbparts/Types/Manifest.hs b/src/Nbparts/Types/Manifest.hs --- a/src/Nbparts/Types/Manifest.hs +++ b/src/Nbparts/Types/Manifest.hs @@ -9,6 +9,7 @@ import Data.Aeson (Options (constructorTagModifier)) import Data.Aeson qualified as Aeson +import Data.Text qualified as Text import Data.Version (Version) import GHC.Generics (Generic) import Paths_nbparts qualified @@ -44,23 +45,32 @@ formatExtension FormatJson = "json" formatExtension FormatMarkdown = "md" -instance Aeson.ToJSON Manifest where - toJSON = Aeson.genericToJSON jsonOptions +instance Aeson.ToJSON Manifest -instance Aeson.FromJSON Manifest where - parseJSON = Aeson.genericParseJSON jsonOptions +instance Aeson.FromJSON Manifest instance Aeson.ToJSON Format where toJSON = Aeson.genericToJSON jsonOptions instance Aeson.FromJSON Format where - parseJSON = Aeson.genericParseJSON jsonOptions + parseJSON = Aeson.withText "Format" $ \case + "yaml" -> pure FormatYaml + "json" -> pure FormatJson + "markdown" -> pure FormatMarkdown + -- Backward compatibility. v0.1.0.0 mistakenly serialised `FormatJson` as + -- "FormatJson" instead of "json", so now we're stuck with dealing with that. + -- For symmetry's sake, might as well include the rest of the formats as well. + "FormatYaml" -> pure FormatYaml + "FormatJson" -> pure FormatJson + "FormatMarkdown" -> pure FormatMarkdown + other -> fail $ "Unknown format: " <> Text.unpack other jsonOptions :: Aeson.Options jsonOptions = Aeson.defaultOptions { constructorTagModifier = \case "FormatYaml" -> "yaml" + "FormatJson" -> "json" "FormatMarkdown" -> "markdown" other -> other } diff --git a/src/Nbparts/Types/Outputs.hs b/src/Nbparts/Types/Outputs.hs --- a/src/Nbparts/Types/Outputs.hs +++ b/src/Nbparts/Types/Outputs.hs @@ -16,10 +16,10 @@ import Nbparts.Types.Mime (UnembeddedMimeBundle) newtype NotebookOutputs a = NotebookOutputs (Map Text [Ipynb.Output a]) -- Map of Cell IDs to outputs. - deriving (Generic, Show, Eq) + deriving (Generic, Show, Eq, Semigroup, Monoid) newtype UnembeddedNotebookOutputs = UnembeddedNotebookOutputs (Map Text [UnembeddedCellOutput]) - deriving (Generic, Show, Eq, Ord) + deriving (Generic, Show, Eq, Ord, Semigroup, Monoid) data UnembeddedCellOutput = Stream diff --git a/src/Nbparts/Unpack.hs b/src/Nbparts/Unpack.hs --- a/src/Nbparts/Unpack.hs +++ b/src/Nbparts/Unpack.hs @@ -67,13 +67,6 @@ Monad.unless cont $ liftIO (Text.hPutStrLn stderr "Operation cancelled: directory not overwritten") Monad.guard cont - let sourceMediaSubdir = "media" - let outputMediaSubdir = "outputs-media" - liftIO $ do - Directory.createDirectoryIfMissing True (exportDirectory sourceMediaSubdir) - Directory.createDirectoryIfMissing True exportDirectory - Directory.createDirectoryIfMissing True (exportDirectory outputMediaSubdir) - -- Parse the notebook. notebookBytes <- liftIO $ LazyByteString.readFile opts.notebookPath (nb :: SomeNotebook) <- @@ -94,17 +87,23 @@ Manifest.outputsFormat = opts.outputsFormat } metadata <- liftEither $ withNb collectMetadata + + let sourceMediaSubdir = "media" (sources, sourceMedia) <- liftEither $ withNb (collectSources sourceMediaSubdir) + + let outputMediaSubdir = "outputs-media" (outputs, outputMedia) <- liftEither $ withNb (collectOutputs outputMediaSubdir) - -- Export manifest, sources, metadata and outputs. + liftIO $ Directory.createDirectoryIfMissing True exportDirectory let yamlOptions = Yaml.setStringStyle nbpartsYamlStringStyle Yaml.defaultEncodeOptions let mkExportPath :: FilePath -> Format -> FilePath mkExportPath fname fmt = exportDirectory fname <.> formatExtension fmt + -- Export manifest. let manifestPath = mkExportPath "nbparts" FormatYaml liftIO $ Yaml.encodeFile manifestPath manifest + -- Export sources. let sourcesPath = mkExportPath "sources" opts.sourcesFormat case opts.sourcesFormat of FormatYaml -> liftIO $ Yaml.encodeFileWith yamlOptions sourcesPath sources @@ -113,20 +112,30 @@ let lang = Maybe.fromMaybe "" $ extractLanguage metadata markdownText <- liftEither $ sourcesToMarkdown lang sources liftIO $ Text.writeFile sourcesPath markdownText - liftIO $ mapM_ (\(path, bytes) -> ByteString.writeFile (exportDirectory path) bytes) sourceMedia + -- Export source media. + liftIO $ Monad.unless (null sourceMedia) $ do + Directory.createDirectoryIfMissing True (exportDirectory sourceMediaSubdir) + mapM_ (\(path, bytes) -> ByteString.writeFile (exportDirectory path) bytes) sourceMedia + + -- Export metadata. let metadataPath = mkExportPath "metadata" opts.metadataFormat liftIO $ case opts.metadataFormat of FormatYaml -> Yaml.encodeFileWith yamlOptions metadataPath metadata FormatJson -> exportJson metadataPath metadata _ -> error $ "Illegal metadata format: " <> show opts.metadataFormat + -- Export outputs. let outputsPath = mkExportPath "outputs" opts.outputsFormat liftIO $ case opts.outputsFormat of FormatYaml -> Yaml.encodeFileWith yamlOptions outputsPath outputs FormatJson -> exportJson outputsPath outputs _ -> error $ "Illegal outputs format: " <> show opts.outputsFormat - liftIO $ mapM_ (\(path, bytes) -> ByteString.writeFile (exportDirectory path) bytes) outputMedia + + -- Export output media. + liftIO $ Monad.unless (null outputMedia) $ do + Directory.createDirectoryIfMissing True (exportDirectory outputMediaSubdir) + mapM_ (\(path, bytes) -> ByteString.writeFile (exportDirectory path) bytes) outputMedia liftIO $ Text.putStrLn ("Unpacked \"" <> Text.pack opts.notebookPath <> "\" to \"" <> Text.pack exportDirectory <> "\"") diff --git a/tests/Tests/Integration/PackSpec.hs b/tests/Tests/Integration/PackSpec.hs --- a/tests/Tests/Integration/PackSpec.hs +++ b/tests/Tests/Integration/PackSpec.hs @@ -2,7 +2,7 @@ import Control.Monad.Except (runExceptT) import Data.Either qualified as Either -import Data.Maybe qualified as Maybe +import Data.Text qualified as Text import Nbparts.Pack ( PackOptions ( PackOptions, @@ -11,7 +11,11 @@ partsDirectory ), ) -import Nbparts.Types (NbpartsError) +import Nbparts.Types (Format + ( FormatJson, + FormatMarkdown, + FormatYaml + ), NbpartsError (UnpackError), renderError) import Nbparts.Unpack ( UnpackOptions ( UnpackOptions, @@ -23,11 +27,11 @@ sourcesFormat ), ) -import Nbparts.Unpack qualified as Unpack +import Nbparts.Unpack.Outputs (collectOutputs) import System.Directory qualified as Directory import System.FilePath (()) import System.IO.Temp (withSystemTempDirectory) -import Test.Hspec (Expectation, Spec, SpecWith, around, context, describe, it, shouldBe, shouldSatisfy) +import Test.Hspec (Spec, SpecWith, around, context, describe, it, shouldBe, shouldSatisfy) import Tests.Integration.Util ( UnpackFormats ( UnpackFormats, @@ -36,6 +40,7 @@ sourcesFormat ), fixtureDir, + readIpynb, runPack, runSpecWithUnpackFormatsCA, runUnpack, @@ -46,63 +51,66 @@ packOutputPath :: Maybe FilePath } -testPackWith :: +runTestUnpackWith' :: UnpackFormats -> - PackTestOptions -> - FilePath -> - (Either NbpartsError () -> Expectation) -> + Maybe FilePath -> FilePath -> - Expectation -testPackWith + IO (Either NbpartsError ()) +runTestUnpackWith' (UnpackFormats {sourcesFormat, metadataFormat, outputsFormat}) - ( PackTestOptions - { unpackOutputPath = relUnpackOutputPath, - packOutputPath = relPackOutputPath - } - ) - fixture - predicate - tmpdir = do - let nbPath = fixtureDir fixture - unpackOutputPath = () tmpdir <$> relUnpackOutputPath + outputPath + nbPath = + runExceptT $ + runUnpack $ + UnpackOptions + { notebookPath = nbPath, + sourcesFormat, + metadataFormat, + outputsFormat, + outputPath = outputPath, + force = False + } - unpackResult <- - runExceptT $ - runUnpack $ - UnpackOptions - { notebookPath = nbPath, - sourcesFormat, - metadataFormat, - outputsFormat, - outputPath = unpackOutputPath, - force = False - } - unpackResult `shouldSatisfy` Either.isRight +runTestUnpackWith :: + UnpackFormats -> + FilePath -> + Maybe FilePath -> + FilePath -> + IO (Either NbpartsError ()) +runTestUnpackWith unpackFmts fixture relOutputPath tmpdir = + runTestUnpackWith' + unpackFmts + (fmap (tmpdir ) relOutputPath) + (fixtureDir fixture) - packResult <- - runExceptT $ - runPack $ - PackOptions - { partsDirectory = Maybe.fromMaybe (Unpack.mkDefOutputPath nbPath) unpackOutputPath, - outputPath = () tmpdir <$> relPackOutputPath, - force = False - } - predicate packResult +runTestPack :: + FilePath -> + Maybe FilePath -> + FilePath -> + IO (Either NbpartsError ()) +runTestPack + relPartsDir + relOutPath + tmpdir = + runExceptT $ + runPack $ + PackOptions + { partsDirectory = tmpdir relPartsDir, + outputPath = () tmpdir <$> relOutPath, + force = False + } runTests :: UnpackFormats -> SpecWith FilePath runTests unpackFormats = do - let testPack = testPackWith unpackFormats - testDefaultOutputPath unpackOutputPath expectedPackOutputPath tmpdir = do - let packTestOpts = - PackTestOptions - { unpackOutputPath = Just unpackOutputPath, - packOutputPath = Nothing - } - testPack - packTestOpts - "empty.ipynb" -- Doesn't really matter what notebook we use. - (`shouldSatisfy` Either.isRight) - tmpdir + let runTestUnpack = runTestUnpackWith unpackFormats + testDefaultOutputPath relPartsDir expectedPackOutputPath tmpdir = do + -- Doesn't really matter what notebook we use. + unpackRes <- runTestUnpack "empty.ipynb" (Just relPartsDir) tmpdir + unpackRes `shouldSatisfy` Either.isRight + + packRes <- runTestPack relPartsDir Nothing tmpdir + packRes `shouldSatisfy` Either.isRight + exists <- Directory.doesFileExist $ tmpdir expectedPackOutputPath exists `shouldBe` True @@ -122,6 +130,35 @@ it "should write a notebook to the path with `.ipynb` appended" $ do _ <- testDefaultOutputPath "test" "test.ipynb" testDefaultOutputPath "test.hello" "test.hello.ipynb" + + context "when given a parts directory with no outputs file" $ + it "should pack as though the notebook has no outputs" $ \tmpdir -> do + unpackRes <- runTestUnpack "stream-outputs.ipynb" (Just "unpacked") tmpdir + unpackRes `shouldSatisfy` Either.isRight + + Directory.removeFile $ + tmpdir + "unpacked" + "outputs." + <> case unpackFormats.outputsFormat of + FormatYaml -> "yaml" + FormatJson -> "json" + FormatMarkdown -> error "Invalid format \"markdown\" for outputs" + + packRes <- runTestPack "unpacked" (Just "repacked.ipynb") tmpdir + packRes `shouldSatisfy` Either.isRight + + eitherNb <- runExceptT $ readIpynb (tmpdir "repacked.ipynb") + let nb = case eitherNb of + Right n -> n + Left err -> error err + + let collected = collectOutputs "outputs-media" nb + outputs = case collected of + Right (o, _media) -> o + Left err -> error (Text.unpack $ renderError (UnpackError err)) + + outputs `shouldBe` mempty spec :: Spec spec = around (withSystemTempDirectory "test-nbparts") $ do diff --git a/tests/Tests/Integration/UnpackSpec.hs b/tests/Tests/Integration/UnpackSpec.hs --- a/tests/Tests/Integration/UnpackSpec.hs +++ b/tests/Tests/Integration/UnpackSpec.hs @@ -20,9 +20,10 @@ sourcesFormat ), ) +import System.Directory qualified as Directory import System.FilePath (()) import System.IO.Temp (withSystemTempDirectory) -import Test.Hspec (Expectation, Spec, SpecWith, around, context, describe, it, shouldBe, shouldSatisfy) +import Test.Hspec (Spec, SpecWith, around, context, describe, it, shouldBe, shouldSatisfy) import Tests.Integration.Util ( UnpackFormats ( UnpackFormats, @@ -35,53 +36,59 @@ runUnpack, ) -testUnpackWith :: UnpackFormats -> FilePath -> (Either NbpartsError () -> Expectation) -> FilePath -> Expectation -testUnpackWith (UnpackFormats {sourcesFormat, metadataFormat, outputsFormat}) fixture predicate tmpdir = do +runTestUnpackWith :: UnpackFormats -> FilePath -> FilePath -> IO (Either NbpartsError ()) +runTestUnpackWith (UnpackFormats {sourcesFormat, metadataFormat, outputsFormat}) fixture tmpdir = do let nbPath = fixtureDir fixture let unpackPath = tmpdir "unpacked" - unpackResult <- - runExceptT $ - runUnpack $ - UnpackOptions - { notebookPath = nbPath, - sourcesFormat, - metadataFormat, - outputsFormat, - outputPath = Just unpackPath, - force = False - } - predicate unpackResult + runExceptT $ + runUnpack $ + UnpackOptions + { notebookPath = nbPath, + sourcesFormat, + metadataFormat, + outputsFormat, + outputPath = Just unpackPath, + force = False + } runTests :: UnpackFormats -> SpecWith FilePath runTests fmts = do - let testUnpack = testUnpackWith fmts + let runTestUnpack = runTestUnpackWith fmts context "when given a notebook with missing cell IDs" $ - it "should return a missing cell ID error" $ - testUnpack "missing-cell-ids.ipynb" $ - shouldBe $ - Left (UnpackError UnpackMissingCellIdError) + it "should return a missing cell ID error" $ \tmpdir -> do + res <- runTestUnpack "missing-cell-ids.ipynb" tmpdir + res `shouldBe` Left (UnpackError UnpackMissingCellIdError) context "when given a malformed notebook" $ - it "should return a parse error" $ - testUnpack "malformed.ipynb" $ \res -> - res `shouldSatisfy` \case - Left (UnpackError (UnpackParseNotebookError _)) -> True - _ -> False + it "should return a parse error" $ \tmpdir -> do + res <- runTestUnpack "malformed.ipynb" tmpdir + res `shouldSatisfy` \case + Left (UnpackError (UnpackParseNotebookError _)) -> True + _ -> False context "when given an empty file" $ - it "should return a parse error" $ - testUnpack "null.ipynb" $ \res -> - res `shouldSatisfy` \case - Left (UnpackError (UnpackParseNotebookError _)) -> True - _ -> False + it "should return a parse error" $ \tmpdir -> do + res <- runTestUnpack "null.ipynb" tmpdir + res `shouldSatisfy` \case + Left (UnpackError (UnpackParseNotebookError _)) -> True + _ -> False context "when given a v3 notebook" $ - it "should return an unsupported notebook error" $ - testUnpack "v3.ipynb" $ \res -> - res `shouldSatisfy` \case - Left (UnpackError (UnpackUnsupportedNotebookFormat (3, 0))) -> True - _ -> False + it "should return an unsupported notebook error" $ \tmpdir -> do + res <- runTestUnpack "v3.ipynb" tmpdir + res `shouldBe` Left (UnpackError (UnpackUnsupportedNotebookFormat (3, 0))) + + context "when given a notebook without attachments or media outputs" $ + it "should not create `media` and `outputs-media` directories" $ \tmpdir -> do + res <- runTestUnpack "empty.ipynb" tmpdir + res `shouldBe` Right () + + mediaExists <- Directory.doesDirectoryExist (tmpdir "unpacked" "media") + mediaExists `shouldBe` False + + outputsMediaExists <- Directory.doesDirectoryExist (tmpdir "unpacked" "outputs-media") + outputsMediaExists `shouldBe` False spec :: Spec spec = around (withSystemTempDirectory "test-nbparts") $ do diff --git a/tests/Tests/Types/ManifestSpec.hs b/tests/Tests/Types/ManifestSpec.hs --- a/tests/Tests/Types/ManifestSpec.hs +++ b/tests/Tests/Types/ManifestSpec.hs @@ -2,6 +2,8 @@ import Control.Arrow (left) import Data.Aeson qualified as Aeson +import Data.Either qualified as Either +import Data.Text.Encoding qualified as Text import Data.Version (Version (Version)) import Data.Yaml qualified as Yaml import Hedgehog (Gen, forAll, tripping, (===)) @@ -19,7 +21,7 @@ currentNbpartsVersion, defManifest, ) -import Test.Hspec (Spec, describe, it) +import Test.Hspec (Spec, describe, it, shouldBe, shouldSatisfy) import Test.Hspec.Hedgehog (hedgehog) genVersion :: Gen Version @@ -76,3 +78,16 @@ it "YAML roundtrip" $ hedgehog $ do fmt <- forAll genFormat tripping fmt Yaml.encode (left (const ()) . Yaml.decodeEither') + + -- v0.1.0.0 mistakenly serialised `FormatJson` as "FormatJson" instead of "json", + -- so we're stuck dealing with that. Although the other formats were serialised correctly, + -- the "wrong" counterparts for them have been added for symmetry. + it "has backward compatibility with v0.1.0.0" $ do + Aeson.eitherDecodeStrict @Format (Text.encodeUtf8 "\"FormatYaml\"") `shouldBe` Right FormatYaml + left (const ()) (Yaml.decodeEither' @Format (Text.encodeUtf8 "\"FormatYaml\"")) `shouldBe` Right FormatYaml + + Aeson.eitherDecodeStrict @Format (Text.encodeUtf8 "\"FormatJson\"") `shouldSatisfy` Either.isRight + left (const ()) (Yaml.decodeEither' @Format (Text.encodeUtf8 "\"FormatJson\"")) `shouldBe` Right FormatJson + + Aeson.eitherDecodeStrict @Format (Text.encodeUtf8 "\"FormatMarkdown\"") `shouldSatisfy` Either.isRight + left (const ()) (Yaml.decodeEither' @Format (Text.encodeUtf8 "\"FormatMarkdown\"")) `shouldBe` Right FormatMarkdown