Skip to content
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

Use AppVeyor artifact URLs in manuscript front-matter #263

Merged
merged 3 commits into from
Aug 30, 2019

Conversation

dhimmel
Copy link
Member

@dhimmel dhimmel commented Aug 9, 2019

Follows up on #262 (comment)
Refs #198

@AppVeyorBot
Copy link

AppVeyor build 1.0.24 for commit 7d1c827 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.24-7d1c827.pdf.

@dhimmel
Copy link
Member Author

dhimmel commented Aug 9, 2019

The updated conda version to have shaved aboutr 32 seconds off build time in terms of environment installation.

@AppVeyorBot
Copy link

AppVeyor build 1.0.25 for commit 0fd41ae by @dhimmel failed.

@AppVeyorBot
Copy link

AppVeyor build 1.0.26 for commit 1f541a8 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.26-1f541a8.pdf.

@dhimmel dhimmel requested a review from agitter August 9, 2019 18:02
Copy link
Member

@agitter agitter left a comment

Choose a reason for hiding this comment

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

The new pull request builds are great for reviewing changes like this.

@@ -12,7 +12,7 @@ _A DOI-citable version of this manuscript is available at <https://doi.org/DOI_H
<small><em>
This manuscript
{% if ci_source is defined -%}
Copy link
Member

Choose a reason for hiding this comment

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

Because ci_source.manuscript_url is created in a separate Manubot function add_manuscript_urls_to_ci_params, should we also ensure that ci_source.manuscript_url is defined here? Or only check for ci_source.manuscript_url?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think ci_params will ever exist without manuscript_url with the current manubot software because:

https://github.com/manubot/manubot/blob/1b885c614634d6e88565ebcf5052b6884132ab32/manubot/process/util.py#L193-L196

But it doesn't hurt to be safe and possibly some users will have an outdated manubot python package with this version of content/00.front-matter.md , so I added the extra check in b07d870

@@ -12,7 +12,7 @@ _A DOI-citable version of this manuscript is available at <https://doi.org/DOI_H
<small><em>
This manuscript
{% if ci_source is defined -%}
([permalink](https://{{ci_source.repo_owner}}.github.io/{{ci_source.repo_name}}/v/{{ci_source.commit}}/))
([permalink]({{ci_source.manuscript_url}}))
{% endif -%}
was automatically generated
{% if ci_source is defined -%}
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me which commit we should show in line 19 below for AppVeyor builds. The manuscript filename contains the triggering commit, but this link contains the merge commit for a pull request build. However, the triggering commit doesn't correspond to the ci_source.repo_slug tree. It resides in the tree of the forked repository.

I don't see a better option that what is already here. I do expect the difference between the filename and link could cause confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is confusing, but I think it's important that the manuscript contains the commit that actually created it.

@AppVeyorBot
Copy link

AppVeyor build 1.0.27 for commit b07d870 by @dhimmel is now complete. The rendered manuscript from this build is temporarily available for download at manuscript-1.0.27-b07d870.pdf.

@dhimmel dhimmel merged commit 6a3ac95 into manubot:master Aug 30, 2019
dhimmel added a commit that referenced this pull request Aug 30, 2019
This build is based on
6a3ac95.

This commit was created by the following Travis CI build and job:
https://travis-ci.com/manubot/rootstock/builds/125258958
https://travis-ci.com/manubot/rootstock/jobs/229678954

The full commit message that triggered this build is copied below:

Use AppVeyor artifact URLs in manuscript front-matter

Merges #263

Update front-matter to use ci_source.manuscript_url
Update environment on 2019-08-09
dhimmel added a commit that referenced this pull request Aug 30, 2019
This build is based on
6a3ac95.

This commit was created by the following Travis CI build and job:
https://travis-ci.com/manubot/rootstock/builds/125258958
https://travis-ci.com/manubot/rootstock/jobs/229678954

The full commit message that triggered this build is copied below:

Use AppVeyor artifact URLs in manuscript front-matter

Merges #263

Update front-matter to use ci_source.manuscript_url
Update environment on 2019-08-09
adebali pushed a commit to CompGenomeLab/lemur-manuscript-archive that referenced this pull request Mar 4, 2020
Merges manubot/rootstock#263

Update front-matter to use ci_source.manuscript_url
Update environment on 2019-08-09
ploegieku added a commit to ploegieku/2023-functional-homology-paper that referenced this pull request Aug 6, 2024
Merges manubot/rootstock#263

Update front-matter to use ci_source.manuscript_url
Update environment on 2019-08-09
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.

3 participants