-
-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX import: create publication without existing index.md #73
Conversation
@ionlights are you able to review this - sounds like it might be another bug introduced by #68 ? |
@christianbrodbeck Just checking - you've run into two errors?
Is that correct? |
1 was an error, 2 is just something that's printed in the console, it does not seem to have any adverse effects (I don't know what to expect to happen with |
@@ -16,9 +16,12 @@ def __init__(self, base_path: Path, delim: str = "---"): | |||
self.path = "" | |||
|
|||
def load(self, file: Path): | |||
self.fm = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be running (at least through academic
) until after the file is made... I think there might be something downstream that's actually going wrong, not this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@christianbrodbeck Can you give some more context about how you ran this?
- Where's your site located?
- Where were you running this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The site is located at ~/Code/christianbrodbeck.net
, which is also the directory from which I ran the command. In case this matters, I'm running macOS 10.15.6, and installed go from the binaries on their website.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the context. 🙂
Yea, if you pass in a command and it's not a known |
When I run the command after applying the fix in this PR then the hugo error seems to get printed once for each publication that's processed, i.e.
|
Hmm... weird. Not sure what those So, @gcushen I see two ways forward with this:
|
So I still had the same terminal in which the error happened open, and I tried to run Does the |
So, if you look at the For this particular case, this entire script generates |
I only found an empty `themes` folder in my repository (very recent clone of starter-academic, just using the default theme), but if there is potentially more work done it would probably make sense to raise a proper error when `hugo` fails instead of the workaround in this PR?
|
@ionlights I propose we remove the dependency on using Hugo to generate the publications (with This proposal may provide some nice benefits for the project:
|
@gcushen All right – at first blush, that means we'll also need to add another flag for the publication directory. The default could be Additionally, I'm not sure portability is as easy as it sounds? (e.g. with Jekyll, if folks wanted sorting of their publications, they need the date formatted in the title, additionally, While I think having this as a generic publication generator would be cool. I'm not sure how well it would hold up in practice – layout logic differs across SSGs. How frequently have you changed the publication archetype in |
@gcushen Just a bump. 🙂 |
@ionlights the publication archetype can be found here: https://github.com/wowchemy/wowchemy-hugo-modules/blob/master/wowchemy/archetypes/publication/index.md . It's history can be viewed on Github by heading to the repo root, choosing a tag, and looking in its previous location at A compromise could be to add a flag to default to using a minimal static built-in template of core front matter parameters (removing the Hugo and Wowchemy dependencies whilst generalising the tool), whilst giving an option for Wowchemy/Academic power users to use the #68 approach of automated This might also enable more comprehensive unit testing without requiring integration (with a Wowchemy site and Hugo) testing to test actual file generation. |
Lol. I forgot to look at Blame. 😂 So, it seems like the archetype has been relatively unchanging. I think that having a core set but allowing folks using Academic to leverage the extras is good. Presumably, we can use the path to infer their Also, the Docker support might need to be dropped. I ran into some interesting issues with I'm not sure what you're getting at about Unit Testing. Can you elaborate on that? |
Great idea, rather than having another CLI flag, we could just check if a We're working on a Wowchemy-specific Docker for GitHub's new online code editor. It will include Go and this Academic CLI. Feel free to contribute: HugoBlox/hugo-blox-builder#1848 As for testing, I mean that really #68 introduces the need for integration testing given the use of |
Moving the outcome of this discussion to a new issue, #85 . Feel free to get involved :) |
When running
% academic import --bibtex ~/pubs.bib
for the first time I got the error below, seemingly because the script was trying to read a non-existingindex.md
file. With this fix I could run the import successfully. I still get theneed to install Go
message despite having installed go (I can runhugo server
), but it does not seem to cause any issues.