Script cleanup #950

Activity

1. florimondmanca

   added

   toolingChanges to our CI/CD, tests setup, etc.Changes to our CI/CD, tests setup, etc.

   on May 15, 2020
2. florimondmanca commented on May 15, 2020

   

   florimondmanca

   on May 15, 2020

   Last edited by florimondmanca

   Contributor

   More actions

   • Agreed with 1/
   • Agreed with 2/ too - scripts/clean sounds less useful now that we publish from CI, where the env is always "clean" anyway. (Also, I personally don't use the clean script.)
   • 3/ A --fix flag is what I've seen in a lot of places (eg in the JS ecosystem). One alternative I thought about is scripts/lint [--check], but scripts/check [--fix] fits my brain better for some reason. Dunno how easy ternary operators are in bash but if most tools have some sort of flag to switch them from "check" to "fix in place", then we could use that. :-)
3. StephenBrown2 commented on May 15, 2020

   

   StephenBrown2

   on May 15, 2020

   Contributor

   More actions

   Chiming in, it probably doesn't matter too much, but I believe linters/formatters most often have a --check flag, which would map down easily from scripts/lint [--check] (Pass down --check to contained programs)

   But it may be beneficial to be non-destructive at first, and require an explicit flag to modify the system.
4. florimondmanca commented on May 15, 2020

   

   florimondmanca

   on May 15, 2020

   Last edited by florimondmanca

   Contributor

   More actions

   It's actually a mix and depends on the tool:

   Tool	Check invocation	Fix invocation
   seed-isort-config	N/A	seed-isort-config
   autoflake	N/A	autoflake --in-place
   flake8	flake8	N/A
   mypy	mypy	N/A
   Black	black --check	black
   isort	isort --check --diff	isort --apply

   So I suppose we could end up with a scripts/check [--fix] something like this…

   #!/bin/sh -e
   
   export PREFIX=""
   if [ -d 'venv' ] ; then
       export PREFIX="venv/bin/"
   fi
   
   export FIX=""; [ "$1" == --fix ] && FIX=true
   export SOURCE_FILES="httpx tests"
   
   BLACK_OPTS="--check --diff"; [ $FIX ] && BLACK_OPTS=""
   ISORT_OPTS="--check --diff"; [ $FIX ] && ISORT_OPTS="--apply"
   
   set -x
   
   if [ $FIX ]; then
       ${PREFIX}seed-isort-config --application-directories=httpx
       ${PREFIX}autoflake --in-place --recursive $SOURCE_FILES
   else
       ${PREFIX}flake8 $SOURCE_FILES
       ${PREFIX}mypy httpx
   fi
   
   ${PREFIX}black $BLACK_OPTS --target-version=py38 $SOURCE_FILES
   ${PREFIX}isort $ISORT_OPTS --project=httpx --recursive $SOURCE_FILES
5. StephenBrown2 commented on May 15, 2020

   

   StephenBrown2

   on May 15, 2020

   Contributor

   More actions

   Ah, I see. black and isort were indeed the formatters I was thinking of, and I knew flake8 didn't fix anything with or without a flag.

   However, that shows that scripts/lint doesn't just replicate scripts/check, but instead runs different things with different flags. Nothing saying that couldn't be implemented as demonstrated though, since it would be only one place to modify a linter or formatter if needed, rather than two.

   Personally I would rename scripts/lint to scripts/format and scripts/check to scripts/lint, but as I said, having the flag is also 👍 .
6. lovelydinosaur

   closed this as completedin #1107on Aug 1, 2020