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

feat(CLI): Yarn-like root scripts fallback #1159

Merged

Conversation

noirbizarre
Copy link
Member

@noirbizarre noirbizarre commented Jun 26, 2022

Pull Request Check List

  • A news fragment is added in news/ describing what is new.
  • Test cases added for changed code.

Describe what you have changed in this PR.

This PR allow to execute PDM scripts without the run command prefix as long as it doesn't conflict with any builtin command or any plugin-provided command.
In case of error, it produces the exact same output, especially in case of shell command not found (ie. instead of having the command not found error from your shell as run does, you still have the PDM usage and error).

In the process I made the "no parameters provided" behavior the same as any others parsing error: usage (help in in this case) is printed on stderr instead of stdout here.

Note: the click.testing.CliRunner have some known issues with stderr (stderr is unbuffered but CliRunner mock it as a buffered one making error message lost in the process) and to be able to test error messages, I had to switch on a simpler home made runner only based on Pytest fixture. Same signature, less click specific in the process but we gain proper stderr handling (as well as the possibility to have the execution output for debug with the RunResult.print() method. I kept the dependency in pyproject.toml to avoid locking in this PR and I wasn't sure about side-effects as some tests install click and having it as a dependency make sure that it is present in the cache.

Note: there has been 3 versions before compatibility with Python 3.7 and Python 3.8 was reached:

  • first one was using argparse's exit_on_error but this was added in Python 3.9
  • second one was trying to backport argparse's exit_on_error, was working but MyPy disagreed
  • this one which now rely on catching SystemExit instead of argparse.ArgumentError: less fine but tests ensure that we don't silently catch other errors

@noirbizarre noirbizarre force-pushed the feature/yarn-like-root-scripts branch from 9f6eb1a to 070e159 Compare June 26, 2022 13:49
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2022

Codecov Report

Merging #1159 (660c2a3) into dev (8b7de33) will increase coverage by 0.04%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##              dev    #1159      +/-   ##
==========================================
+ Coverage   85.41%   85.46%   +0.04%     
==========================================
  Files          78       78              
  Lines        6790     6803      +13     
  Branches     1555     1558       +3     
==========================================
+ Hits         5800     5814      +14     
+ Misses        659      658       -1     
  Partials      331      331              
Flag Coverage Δ
unittests 85.22% <86.66%> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
pdm/core.py 90.98% <86.66%> (+1.99%) ⬆️

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 8b7de33...660c2a3. Read the comment docs.

@noirbizarre noirbizarre force-pushed the feature/yarn-like-root-scripts branch from 4547394 to 45e377e Compare June 26, 2022 15:37
@frostming
Copy link
Collaborator

This PR allow to execute PDM scripts without the run command prefix as long as it doesn't conflict with any builtin command or any plugin-provided command.

I would prefer to restrict it to user scripts only, so pdm flask run shouldn't work.

@frostming frostming added this to the Release 2.0 milestone Jun 27, 2022
@noirbizarre noirbizarre force-pushed the feature/yarn-like-root-scripts branch from 45e377e to 57fb07b Compare June 27, 2022 10:45
@noirbizarre
Copy link
Member Author

I hesitated on this one.
Updated to restrict to user scripts only ✅

@noirbizarre noirbizarre force-pushed the feature/yarn-like-root-scripts branch from 1f98450 to 660c2a3 Compare June 27, 2022 12:42
Copy link
Collaborator

@frostming frostming left a comment

Choose a reason for hiding this comment

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

LGTM, also thanks for your detailed technical explanation.

@noirbizarre
Copy link
Member Author

You're welcome 🙏🏼
Do you know why sometimes a python version is missing on test_integration::test_bsaic_integration ? Do you think it's something we can fix somehow or is it only a github worker issue ?

@frostming
Copy link
Collaborator

You're welcome 🙏🏼 Do you know why sometimes a python version is missing on test_integration::test_bsaic_integration ? Do you think it's something we can fix somehow or is it only a github worker issue ?

Yeah, I just don't have the energy to dig it, it seems quite flaky. I doubt it is related to the GitHub Action tools cache.

@frostming frostming merged commit edd4559 into pdm-project:dev Jun 28, 2022
frostming added a commit that referenced this pull request Jun 28, 2022
* feat(core): Use tomllib on Python 3.11 (#1072)

* docs: 📝 Fix typo in `pip install pdm` description (#1061)

* Use tomllib on Python 3.11

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* news

* use a compatibility module

* missed one import

Co-authored-by: t106362512 <[email protected]>
Co-authored-by: hauntsaninja <>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* feat(core): Replace halo, click, and termcolor with rich (#1091)

* feat(core): Use `unearth` as the backend to find and download packages (#1096)

* perf(resolver): Speed up the resolution with lazy find_matches (#1098)

* Improve the output of installing packages

* Use confirm instead of ask

* feat(core): New command: pdm publish (#1107)

* Use rich handler for logging to stderr

* feat(scripts): added composite tasks support (#1117)

* feat(core): Add option to skip hooks (#1127)

* fix(scripts): allow pdm test command to receive path arguments

* feat(hooks): added a `--skip` option to skipp scripts and hooks (#1127)

fix #948

* feat(hooks): use PDM_SKIP_HOOKS environement variable as fallback for skip list

* feat(core): Support setup.py import (#1137)

* Update completion script

* fix(resolution): fix a bug that versions with local part can't be found and installed
Close #1093

* feat(core): forbid editable depenencies in project table (#1140)

* Make the error message more friendly

* doc: improve the docs about dependencies

* doc: add CLI reference doc

* doc: use asciiart as the program description

* chore: remove remaining artifacts from #1127 (#1152)

* Feature: complete lifecycle signals and documentation (#1147)

* feat(hooks): Added pre-publish hook

* refactor(hooks): dynamic signal/hooks listing avoiding double declaration

* feat(hooks): added (pre|post)_script and (pre|post)_run hooks

* doc(hooks): added lifecycle and hooks documentation

* review fix

* fix(tests): add and use the _echo fixture for cross-plateform and concise test echos

* refactor(hooks): automatically register the script handler for all hooks

* feat: Update pdm-pep517 to 1.0 (#1153)

* fix(scripts): merge the Script and Description field from listing (#1151)

* feat: fetch the candidate hashes concurrently (#1154)

* feat: fetch the candidate hashes concurrently

* add news

* Feat/respect-source-order (#1155)

* doc: restructure the docs about project metadata and build configuration

* parse pep 621 metadata to avoid build (#1156)

* feat: Remove the compatible support for pdm legacy metadata (#1157)

* fix(config): use platform standard directories for all PDM directories (#1161)

Fixes #1150

* fix(#1156): only trust parsing result when all are static

* New build configuration table

* chore: added a tox.ini file for easier local testing against all Python versions (#1160)

* feat(CLI): Yarn-like root scripts fallback (#1159)

* feat(hooks): added a post_use hook (#1163)

Co-authored-by: Shantanu <[email protected]>
Co-authored-by: t106362512 <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Axel H <[email protected]>
@noirbizarre noirbizarre deleted the feature/yarn-like-root-scripts branch June 28, 2022 08:45
@noirbizarre
Copy link
Member Author

I'll take a look if I have time

@frostming
Copy link
Collaborator

Besides, it randomly errors

unearth: Skip https://pypi.org/simple/pytz/ because of NotHTML: only HTML is   
E       supported but its content-type is ..

I don't know why the integration test is so flaky. May need some improvement on the test architecture of integration tests.

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