Skip to content

Conversation

@rhubert
Copy link
Contributor

@rhubert rhubert commented Feb 17, 2023

No description provided.

@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Patch coverage: 95.45% and project coverage change: -0.36 ⚠️

Comparison is base (a52267f) 88.26% compared to head (a675e67) 87.90%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
- Coverage   88.26%   87.90%   -0.36%     
==========================================
  Files          46       46              
  Lines       14370    14400      +30     
==========================================
- Hits        12683    12658      -25     
- Misses       1687     1742      +55     
Impacted Files Coverage Δ
pym/bob/input.py 91.91% <95.45%> (+0.01%) ⬆️
pym/bob/generators/__init__.py 80.00% <0.00%> (-20.00%) ⬇️
pym/bob/share.py 84.71% <0.00%> (-3.50%) ⬇️
pym/bob/cmds/misc.py 90.67% <0.00%> (-2.60%) ⬇️
pym/bob/utils.py 85.79% <0.00%> (-2.47%) ⬇️
pym/bob/invoker.py 84.59% <0.00%> (-2.09%) ⬇️
pym/bob/languages.py 88.77% <0.00%> (-1.07%) ⬇️
pym/bob/audit.py 91.43% <0.00%> (-0.69%) ⬇️
pym/bob/builder.py 92.21% <0.00%> (-0.54%) ⬇️
pym/bob/generators/common.py 76.98% <0.00%> (-0.40%) ⬇️
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@jkloetzke jkloetzke left a comment

Choose a reason for hiding this comment

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

I think there is a fundamental problem with the handling of provideDeps. With the current approach the variable expansion will be done after the pattern expansion. This is exactly the opposite order in which shells are doing it. Think about patterns like *-${FOO}-dev. This won't ever match anything. I think the order has to be reversed to be useful...

@rhubert rhubert force-pushed the 317 branch 3 times, most recently from 48383c4 to 9516691 Compare February 19, 2023 08:39
@rhubert
Copy link
Contributor Author

rhubert commented Feb 19, 2023

At least providedDeps are still broken... I'll look into this later...

@rhubert
Copy link
Contributor Author

rhubert commented Feb 19, 2023

I think the provideDeps issue is now solved. I squashed all the commits - please comment the latest version ;)

@jkloetzke
Copy link
Member

I think the code works as intended. However, I took myself the freedom to optimize it a bit so that the performance overhead is not noticeable. Previously the recipe parsing time was ~8% longer, even if the feature was not used. Now it's somewhere at 0.3%.

Once the documentation is updated it's ready for merging I guess...

@rhubert
Copy link
Contributor Author

rhubert commented Feb 27, 2023

Thanks!!!

I did some testing this morning and it's still working for my use-cases ;)

Added a few words to the documentation + squashed my commits.

@rhubert rhubert marked this pull request as ready for review February 27, 2023 11:27
rhubert and others added 3 commits February 28, 2023 20:38
Apply string substitution to the names of dependencies. This avoids the
need of duplication and/or use of `if`.
Most likely no string substitution is required on provideDeps. Wrap the
entries into a specialized class that handles only what is really
necessary.
@jkloetzke jkloetzke merged commit a41e823 into BobBuildTool:master Mar 1, 2023
@jkloetzke
Copy link
Member

Thanks!

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