{"comment":{"url":"https://api.github.com/repos/MelLain/bigartm/comments/15209810","html_url":"https://github.com/MelLain/bigartm/commit/84659becd93ccf661b0134592ed9b726f88eb1e4#commitcomment-15209810","id":15209810,"user":{"login":"sashafrey","id":1252606,"avatar_url":"https://avatars.githubusercontent.com/u/1252606?v=3","gravatar_id":"","url":"https://api.github.com/users/sashafrey","html_url":"https://github.com/sashafrey","followers_url":"https://api.github.com/users/sashafrey/followers","following_url":"https://api.github.com/users/sashafrey/following{/other_user}","gists_url":"https://api.github.com/users/sashafrey/gists{/gist_id}","starred_url":"https://api.github.com/users/sashafrey/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sashafrey/subscriptions","organizations_url":"https://api.github.com/users/sashafrey/orgs","repos_url":"https://api.github.com/users/sashafrey/repos","events_url":"https://api.github.com/users/sashafrey/events{/privacy}","received_events_url":"https://api.github.com/users/sashafrey/received_events","type":"User","site_admin":false},"position":3,"line":136,"path":"src/artm/core/helpers.cc","commit_id":"84659becd93ccf661b0134592ed9b726f88eb1e4","created_at":"2015-12-30T17:50:45Z","updated_at":"2015-12-30T17:50:45Z","body":"I think this require a bit of cleanup. Do you know if the `batch` here is expected to have an ID or not? Now sure? That's a problem... Look up for all usages of SaveBatch, there are not that many... I think most callers don't bother to set batch ID, and the batch is actually saved without an ID. Then there is a logic in LoadBatch method that generates the ID from the filename (assuming that filename is a GUID). LoadBatch will throw an exception when batch has no ID, and its not possible to parse ID from filename. \r\n\r\nTo summarize, you need always save batches with an appropriate `id` that represents a GUID. Best way is to \r\n* change this method to require `name` argument\r\n* check that the batch has ID, and throw an exception if it doesn't\r\n* go through all callers and make sure they set an ID; use the ID as filename\r\n* for your specific case when `SaveBatch` is called from `CollectionParser`, and when `NameType==code`, use your `BatchNameGenerator` as a file name (but make sure that `batch.id` is set to a GUID)\r\n\r\n\r\n\r\n\r\n Why would SaveBatch generate an UUID when `name` is emtpy? You don't use it afterwards...\r\nBest option would be to split into two methods:\r\n```\r\nSaveBatch(const Batch& batch, const std::string& disk_path);\r\nSaveBatch(const Batch& batch, const std::string& disk_path, const std::string& name);\r\n```\r\nFirst one works as today, second just calls ``SaveMessage(file.string(), disk_path, batch)``.\r\n"}}