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

Add a workflow for nitpicking on trailing spaces and similar issues (to discuss) #892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ghost
Copy link

@ghost ghost commented Jun 15, 2021

PR creator: Description

Adds a GitHub workflow to find when commits add:

  • trailing spaces (and trailing protected spaces)
  • tabs
  • Windows/Mac line ends
  • long lines (lines outside of a screen that are longer than 90 characters -- 90 characters gives some leeway over the normal 80 characters)

Caveats etc.:

* This is the hackiest possible implementation, if we agree to do this,
  it would probably be best to use an own GitHub Action action for it.
* I wanted to use "continue-on-error: true" in my steps but while that
  correctly recognizes all issues, people would not see the evil red
  X marking the check failed (which arguably we want to have).
* Output is not particularly pretty currently.
* On longer PRs, the current output may be a bit confusing, because we
  don't try to give file names/lines that failed.
* This only runs on PRs. Running on push would mean running after the
  damage is already dones.
* This will only compare main..my-branch, rather than each commit added
  to my-branch individually
* Checking out the 1.3 GB doc-sle repo for 10 seconds worth of scripts
  is a bit wasteful. Unfortunately, it would not work easily with any
  kind of `git clone --depth` setting. :/

PR creator: Which product versions do the changes apply to?

When opening a PR, check all versions of the documentation that your PR applies to.

  • SLE 15/openSUSE Leap 15.x
    • SLE 15 SP4/openSUSE Leap 15.4 (if applicable to 15 SP4 only -- do not merge yet!)
    • SLE 15 SP3/openSUSE Leap 15.3 (current main, no backport necessary)
    • SLE 15 SP2/openSUSE Leap 15.2
    • SLE 15 SP1
    • SLE 15 GA
  • SLE 12
    • SLE 12 SP5
    • SLE 12 SP4
    • SLE 12 SP3

PR reviewer only: Have all backports been applied?

The doc team member merging your PR will take care of backporting to older documents.
When opening a PR, do not set the following check box.

  • all necessary backports are done

@ghost ghost force-pushed the feature/workflow-nitpicks branch 5 times, most recently from 5a4f490 to b2294c9 Compare June 15, 2021 14:16
@ghost ghost marked this pull request as draft June 15, 2021 14:18
@ghost ghost force-pushed the feature/workflow-nitpicks branch 4 times, most recently from 8506c49 to 8f8f31e Compare June 15, 2021 15:07
@ghost ghost changed the title Feature/workflow nitpicks Add a workflow for nitpicking on trailing spaces and similar issues (to discuss) Jun 15, 2021
@ghost ghost marked this pull request as ready for review June 15, 2021 15:13
@ghost ghost force-pushed the feature/workflow-nitpicks branch 4 times, most recently from 50e7a5d to 6ebeb6e Compare June 15, 2021 17:02
Copy link
Contributor

@tomschr tomschr left a comment

Choose a reason for hiding this comment

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

Thanks, looks good! 👍 I have some minor comments:

  • As shown below, I'd really suggest to use the ::error::{message} template to make error messages consistent and distinct from other debug messages.

  • You can avoid the exit commands (see my comment below). Would reduce the line count a bit.

  • If you really want to keep the exit commands, maybe reduce it like this:

      if [ ... ]; then
          echo "::error::Something bad happens"
          exit 1
      fi
      echo "All fine"
    

Hope this helps. 🙂

Comment on lines +29 to +30
echo -e "\nThis pull request introduces tabs (→) on the following lines:"
echo -e "\n$tab\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest to use the GitHub formatted error message to make it visually distinct from other messages:

https://docs.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message

Syntax

::error file={name},line={line},col={col}::{message}

Most of these parameters are optional. So you would probably use something like this:

Suggested change
echo -e "\nThis pull request introduces tabs (→) on the following lines:"
echo -e "\n$tab\n"
echo "::error::This pull request introduces tabs (→) on the following lines:"
echo "::error::$tab"

You may want to apply it to the other echo commands as well.

Copy link
Author

Choose a reason for hiding this comment

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

An exit command isn't really necessary if you use the template from previous item. The Action will stop this step anyway.

If that is true, then using two echos with ::error:: in succession as suggested here won't work, because it'd exit after the first ::error:: message. I am also unsure if ::error:: allows for -e which is extremely necessary for the second echo.

Additionally, an exit just blows up the code for my taste.

exit 1; is 7 characters ... if anything "blows up the code", it's hardly that.^^

You can use the continue-on-error: true. What you need is to "summarize" the result of each steps into an output. Then in a last step you check each one and react accordingly

I should probably do something like that. It could have been less work though if continue-on-error worked better though. :/

Copy link
Contributor

Choose a reason for hiding this comment

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

An exit command isn't really necessary if you use the template from previous item. The Action will stop this step anyway.

If that is true, then using two echos with ::error:: in succession as suggested here won't work, because it'd exit after the first ::error:: message. I am also unsure if ::error:: allows for -e which is extremely necessary for the second echo.

Right, maybe I misremembered that.

Additionally, an exit just blows up the code for my taste.

exit 1; is 7 characters ... if anything "blows up the code", it's hardly that.^^

It's not only this line, there are other lines. But it's probably a matter of taste, so ignore that. 😉

I should probably do something like that. It could have been less work though if continue-on-error worked better though. :/

Right. It was just an idea. As you wrote, maybe for the future it would be better anyway to move it into a "real" GH Action. But for the time being, you could leave it as is.

@tomschr
Copy link
Contributor

tomschr commented Jun 22, 2021

Regarding this item:

I wanted to use "continue-on-error: true" in my steps but while that correctly recognizes all issues, people would not see the evil red X marking the check failed (which arguably we want to have).

You can use the continue-on-error: true. What you need is to "summarize" the result of each steps into an output. Then in a last step you check each one and react accordingly. Here is a rough snippet:

jobs:
  nitpicks:
    steps:
      - name: Step 1
        id: step1
        run: |
           # ... $RESULT can be 0 or 1
           echo "::set-output name=result::$RESULT"

      - name: Step 2
        id: step2
        run: |
           # ... $RESULT can be 0 or 1
           echo "::set-output name=result::$RESULT"

     # many more steps in between
     - name: Collect result from each step
       run: |
          if [[ $(( ${{ steps.step1.outputs.result }} + ${{ steps.step2.outputs.result }} )) -ge 1 ]]; then
            # ... maybe output something useful
            exit 1
          fi

(Haven't tested it yet, but I think it should work.)

@ghost ghost force-pushed the feature/workflow-nitpicks branch from 6ebeb6e to 26831b3 Compare January 20, 2022 22:30
* This is the hackiest possible implementation, if we agree to do this,
  it would probably be best to use an own GitHub Action action for it.
* I wanted to use "continue-on-error: true" in my steps but while that
  correctly recognizes all issues, people would not see the evil red
  X marking the check failed.
* Output is not particularly pretty currently.
* On longer PRs, the current output may be a bit confusing, because we
  don't try to give file names/lines that failed.
* This only runs on PRs. Running on push would mean running after the
  damage is already dones.
* This will only compare main..my-branch, rather than each commit added
  to my-branch individually
* Checking out the 1.3 GB doc-sle repo for 10 seconds worth of scripts
  is a bit wasteful. Unfortunately, it would not work easily with any
  kind of --depth setting. :/
* This workflow fails the line length criteria too.
@ghost ghost force-pushed the feature/workflow-nitpicks branch from 26831b3 to e169312 Compare January 28, 2022 19:48
@ghost
Copy link
Author

ghost commented Feb 10, 2022

@tomschr @taroth21 I am wondering what to do with this now.

  • I will not get to finishing this and making this better. (Big issue: It does not tell you which files are failing.)
  • I am worrying that it might ultimately just have the effect of people ignoring CI altogether because they will see a red icon very often, despite the documentation building. (I think many people are still not used to looking at the CI error log and think CI is a complete black box.) So this workflow might achieve the opposite of its stated goal.

@tomschr
Copy link
Contributor

tomschr commented Feb 10, 2022

@sknorr
I was curious and looked at the marketplace and voilá! For the trailing whitespace issue, there is already an existing Action: https://github.com/marketplace/actions/find-trailing-whitespace Could serve as inspiration?

Although this Action would be nice, I think it isn't something of uttermost importance. You don't need to finish this. Just leave it open for the time being and we can come back later. We could discuss it for the next SLE writers call.

@ghost
Copy link
Author

ghost commented Feb 10, 2022

https://github.com/marketplace/actions/find-trailing-whitespace

The implementation there is ... lacking, even compared to what I have:

  • It checks all files, rather than just the diffs (in a big project like doc-sle, this means that the first person to run this tool will be staring into a firehose of thousands of issues)
  • Despite that simple approach, it does not output the name of the file or the line number with the trailing spaces
  • It really only checks trailing spaces, nothing else.

I am pretty sure that there is an existing action that is much better than mine, but this is not the one. :)

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.

1 participant