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.

Sign up for GitHub

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

Jump to bottom

Fix tasks.do-copy-apidocs #444

Merged
merged 1 commit into from Aug 14, 2020
Merged

Fix tasks.do-copy-apidocs #444

merged 1 commit into from Aug 14, 2020

Conversation

haraldh
Copy link
Contributor

@haraldh haraldh commented Aug 7, 2020

  1. It didn't work with a non-default target specified e.g. in
    .cargo/config
  2. It didn't work with CARGO_MAKE_CRATE_WORKSPACE_MEMBERS
  3. It didn't work when running cargo make docs-flow multiple times

@codecov-commenter
Copy link

codecov-commenter commented Aug 7, 2020 •
edited

Codecov Report

Merging #444 into 0.32.2 will increase coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           0.32.2     #444      +/-   ##
==========================================
+ Coverage   92.61%   92.77%   +0.16%     
==========================================
  Files          91       91              
  Lines       17635    17639       +4     
==========================================
+ Hits        16333    16365      +32     
+ Misses       1302     1274      -28
Impacted Files Coverage Δ
src/lib/descriptor/makefiles/mod_test.rs 98.11% <100.00%> (ø)
src/lib/installer/crate_installer_test.rs 90.17% <0.00%> (-1.16%) ⬇️
src/lib/installer/cargo_plugin_installer_test.rs 93.10% <0.00%> (-1.15%) ⬇️
src/lib/cli.rs 89.14% <0.00%> (-0.41%) ⬇️
src/lib/runner.rs 88.17% <0.00%> (-0.29%) ⬇️
src/lib/environment/mod_test.rs 96.16% <0.00%> (+0.57%) ⬆️
src/lib/runner_test.rs 90.84% <0.00%> (+1.00%) ⬆️
src/lib/descriptor/mod_test.rs 96.14% <0.00%> (+1.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2effff0...71d7d1c. Read the comment docs.

@sagiegurari
Copy link
Owner

thanks a lot for the PR.
few comments: you are not using the doc root folder env var. any reason for that?
what if a project would like to change it?
second, this won't work on windows, so if you want to add conditions and such, we should convert it to duckscript maybe.

@haraldh
Copy link
Contributor Author

haraldh commented Aug 7, 2020 •
edited

with CARGO_MAKE_CRATE_WORKSPACE_MEMBERS the CARGO_MAKE_DOCS_ROOT_FOLDER does not point to the current crate the docs were built when doing cargo docs-flow

@sagiegurari
Copy link
Owner

you mean when the project is a workspace? in that case, i guess the best fix is to set it up correctly for those type of projects as well.
if scripting is a must, than it can be via duckscript because I'm not sure how else it would be cross platform

@haraldh
Copy link
Contributor Author

haraldh commented Aug 10, 2020

Ok, here is my take on it with duckscript.

@sagiegurari
Copy link
Owner

Thanks a lot. looks promising. i'm going to locally test it as well. probably tomorrow.

sagiegurari
sagiegurari requested changes Aug 14, 2020
View reviewed changes
Copy link
Owner

@sagiegurari sagiegurari left a comment

Choose a reason for hiding this comment

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

I tested it and it seems to work great, thanks a lot.
lets do few minor changes and then i'll push it in.
i want it to be part of the next release.

src/lib/descriptor/makefiles/rust.toml Outdated
SOURCE_DIRECTORY=${CARGO_MAKE_DOCS_ROOT_FOLDER}/target/doc
TARGET_DIRECTORY=${CARGO_MAKE_DOCS_ROOT_FOLDER}/docs/api/${CARGO_MAKE_DOCS_SUB_FOLDER}
if is_empty ${CARGO_MAKE_DOCS_SUB_FOLDER}
SOURCE_DIRECTORY= set ${CARGO_MAKE_DOCS_ROOT_FOLDER}
Copy link
Owner

Choose a reason for hiding this comment

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

since SOURCE_DIRECTORY is now actually a variable, can we name it source_dir or something short?
same goes for other variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

src/lib/descriptor/makefiles/rust.toml Outdated

echo Source Directory: ${SOURCE_DIRECTORY}
echo Target Directory: ${TARGET_DIRECTORY}

mkdir -p ${TARGET_DIRECTORY}
mv ${SOURCE_DIRECTORY}/* ${TARGET_DIRECTORY}
if is_dir ${TARGET_DIRECTORY}
Copy link
Owner

Choose a reason for hiding this comment

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

no need to check if the dir exists. you can just remove. it won't break if it doesn't exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

src/lib/descriptor/makefiles/rust.toml Outdated
SOURCE_DIRECTORY= set ${SOURCE_DIRECTORY}/target/doc
end

if is_empty ${CARGO_MAKE_DOCS_SUB_FOLDER}
Copy link
Owner

Choose a reason for hiding this comment

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

just in case to make sure CARGO_MAKE_DOCS_SUB_FOLDER is defined, let change to:
if is_empty "${CARGO_MAKE_DOCS_SUB_FOLDER}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

@sagiegurari sagiegurari changed the base branch from master to 0.32.2 August 14, 2020 13:11
@haraldh
Fix tasks.do-copy-apidocs
71d7d1c 
1. It didn't work with a non-default target specified e.g. in
   `.cargo/config`
2. It didn't work with `CARGO_MAKE_CRATE_WORKSPACE_MEMBERS`
3. It didn't work when running `cargo make docs-flow` multiple times
@sagiegurari
Copy link
Owner

thanks a lot for the PR. merging

@sagiegurari sagiegurari merged commit b3593da into sagiegurari:0.32.2 Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagiegurari sagiegurari Awaiting requested review from sagiegurari

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@haraldh @codecov-commenter @sagiegurari