Skip to content

Implementation of build steps and support for old API #32

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

Merged
merged 17 commits into from
Mar 14, 2022

Conversation

axtimhaus
Copy link
Contributor

@axtimhaus axtimhaus commented Feb 11, 2022

Changes

Todo

  • Check if tests must be changed and/or extended (current tests passing)
  • update docs

@tobiasraabe tobiasraabe added this to the v0.1.2 milestone Feb 21, 2022
@tobiasraabe
Copy link
Member

Hi @axtimhaus, this looks awesome! Can I offer any help or are you just looking for some time to finish the implementation?

@axtimhaus
Copy link
Contributor Author

If you would check up the tests I would be thankful ;)
I'm going to build the other build steps soon.

@codecov
Copy link

codecov bot commented Mar 2, 2022

Codecov Report

Merging #32 (56321d5) into main (a114ce1) will decrease coverage by 0.25%.
The diff coverage is 98.44%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #32      +/-   ##
==========================================
- Coverage   98.85%   98.60%   -0.26%     
==========================================
  Files          14       17       +3     
  Lines         524      572      +48     
==========================================
+ Hits          518      564      +46     
- Misses          6        8       +2     
Flag Coverage Δ
end_to_end 92.83% <98.44%> (+4.47%) ⬆️
unit 59.61% <52.71%> (-7.37%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
tests/test_collect.py 97.72% <ø> (-1.09%) ⬇️
src/pytask_latex/collect.py 97.67% <94.87%> (-2.33%) ⬇️
src/pytask_latex/compilation_steps.py 100.00% <100.00%> (ø)
src/pytask_latex/parametrize.py 100.00% <100.00%> (ø)
src/pytask_latex/path.py 100.00% <100.00%> (ø)
src/pytask_latex/utils.py 100.00% <100.00%> (ø)
tests/test_execute.py 100.00% <100.00%> (ø)
tests/test_parallel.py 97.46% <100.00%> (+1.10%) ⬆️
tests/test_parametrize.py 98.21% <100.00%> (+0.53%) ⬆️

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 a114ce1...56321d5. Read the comment docs.

@tobiasraabe
Copy link
Member

Hi @axtimhaus,

I got the tests running and made some changes to the code. Here is some feedback:

  • I had to remove the generator approach for the build steps since task functions are pickled during parallelization and generators are not pickable.
  • During development, try to keep the tests passing. Otherwise, you might later need to fix many bugs at once.
  • Moreover, I found it helpful to, first, implement tests for new features and, secondly, the feature. The tests ensure the correctness of the implementation and help you to focus on the necessary changes.

@axtimhaus
Copy link
Contributor Author

Hi @tobiasraabe,

yes, you are right, I lack a bit of discipline in that stuff...
I often work with code that is not testable very well, so I have to get used to it.
But I ran the tests after my changes, and all existing ones were passing. Maybe something went wrong, have to check this.

So could I get over to implement the other steps?

@tobiasraabe
Copy link
Member

No problem. If you like you can move over implementing the other build steps.

I'll wrap up this PR probably on Monday and document the new interface.

@tobiasraabe
Copy link
Member

Hi @axtimhaus, I finished the readme. Just one thing which might sound silly. If people want to abbreviate the import for the build steps, they might do from pytask_latex import build_steps as bs. You can call it a namespace collision. Would you be fine with renaming it to compilation_steps?

@tobiasraabe tobiasraabe merged commit ed179b5 into pytask-dev:main Mar 14, 2022
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