Skip to content

Fix loading repo name from yaml - #63

Merged
eshellman merged 3 commits into
masterfrom
book_post_fix
Mar 26, 2018
Merged

Fix loading repo name from yaml#63
eshellman merged 3 commits into
masterfrom
book_post_fix

Conversation

@bdr99

@bdr99 bdr99 commented Feb 28, 2018

Copy link
Copy Markdown
Contributor

The addBookFromYaml function was not loading the repo name into the database, so this PR includes a fix for this issue.

@bdr99 bdr99 mentioned this pull request Feb 28, 2018
Comment thread gitensite/apps/bookinfo/db.py Outdated

(book,created) = Book.objects.get_or_create(book_id=int(obj['identifiers']['gutenberg']))

if "repo_name" in obj:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The repo_name property comes the load_repos management command, where I add it to the end of the YAML data. See the changes to load_repos.py in this PR. This is how I pass the repo name to the addBookFromYaml function. There doesn't appear to be a "_repo" property that exists on the object created by the BookMetadata constructor. Let me know if I am misunderstanding what you are saying.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gitenberg.util.catalog.BookMetadata objects are meant to be constructed with gitenberg.book.Book objects. The constructor for these objects takes repo_name as a parameter, if not, it generates the reponame. Your load_repos is constructing with it gitensite.apps.bookinfo.models.Book objects. If you want to duck type it, you need your Book object to mimic the behaviour of the gitenberg.book.Book object.

@eshellman

Copy link
Copy Markdown
Contributor

I think it's BookMetadata.metadata._repo

metadata=BookMetadata(Book(book_id=pg_id), rdf_library=rdf_library, enrich=should_enrich)

# Add repo_name to yaml
yaml = metadata.__unicode__() + "\nrepo_name: " + repo_name

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is error-prone in a number of ways. Better to do
metadata.metadata["_repo"] = repo_name


# Add repo_name to yaml
yaml = metadata.__unicode__() + "\nrepo_name: " + repo_name
addBookFromYaml(yaml)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that your code takes a dict, converts it to a yaml string, and then the first line in addBookFromYaml converts the yaml string into a dict

@bdr99 bdr99 Mar 19, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can see the issue here. I will work on restructuring this.

@bdr99 bdr99 mentioned this pull request Mar 25, 2018
@bdr99

bdr99 commented Mar 25, 2018

Copy link
Copy Markdown
Contributor Author

OK, now we have changed our approach for the load_repos command and how the repo name gets passed to the addBookFromYaml command. The load_repos command now passes the BookMetadata object directly to the addBookFromYaml command after adding the _repo property to its metadata dictionary.

@eshellman
eshellman merged commit 2c1c657 into master Mar 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants