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

move pre-commit hook back to travis #3195

Closed
wants to merge 2 commits into from

Conversation

daschuer
Copy link
Member

Unfortunately the output of the pre-commit hook on GitHub is only usable on BIG screens.
Travis did a good job in the past, so let's move back until we have a solution on GitHub.

@daschuer
Copy link
Member Author

daschuer commented Oct 21, 2020

Here how it looks on my HD-ready screen:
grafik

Scaling it down using CTRL- makes the console output is unreadable.

@Holzhaus
Copy link
Member

On my 15.4" 1080p laptop screen, it looks like this:

Screenshot from 2020-10-21 14-11-45

Can't you scroll down? Or click on the hamburger menu in the right top corner -> "Show full screen"?

In that case it looks like this:
Screenshot from 2020-10-21 14-13-10

@daschuer
Copy link
Member Author

Ok, I had not yet discovered "Show full screen".
That works.

But this is still no solution for the phone, where it switches back to desktop view. Also all the hide buttons are still annoying.

The Travis experience is much more pleasant compared to that.

For my understanding we have switched to GitHub for an improved user experience, but the opposite is the case.

What is the drawback to just switch back?

@Holzhaus
Copy link
Member

But this is still no solution for the phone, where it switches back to desktop view.

The Full screen button works on my phone.

Also all the hide buttons are still annoying.

I actually like them a lot, because that way I don't need to scroll down so much and my phone doesn't slow down due to hundres/thousands of lines.

The Travis experience is much more pleasant compared to that.

I see your points, and the experience is not perfect. But I still like it a lot better than Travis, which has problems of its own. Slowdowns due to log loading (in contrast to lazy loading on GH Actions) and the non-working back buttons is quite annoying.

For my understanding we have switched to GitHub for an improved user experience, but the opposite is the case.

One of the reasons was visibility to first-time contributors, yes. But I also proposed the switch to clear up the Travis Build queue, so that pre-commit jobs don't block build jobs and increases the waiting time.

@Holzhaus
Copy link
Member

@mixxxdj/continuous-integration-managers What do the others think?

@Holzhaus
Copy link
Member

Idea: I could also look into checking the output, and write a bot thwt comments on the PR if the pre-commit hook fails. That comment could explain how to install/use the precommit hooks and a link to a gist with the patch diff and how to apply that diff using git apply.

@daschuer
Copy link
Member Author

But this is still no solution for the phone, where it switches back to desktop view.

The Full screen button works on my phone.

Did you see a mobile view? I can also switch to full screen but the text is not readable without a magnifier glass. I use Chrome on Android.

Also all the hide buttons are still annoying.

I actually like them a lot, because that way I don't need to scroll down so much and my phone doesn't slow down due to hundrets/thousands of lines.

This is not the case on Travis for the pre-commit hook. It also hides relevant lines and reduces the output from 1378 to 80 lines.
It performer this in place, which is much more pleasant to me.
In addition it has the "Scroll to the end of the Log" button.
See: https://travis-ci.org/github/mixxxdj/mixxx/jobs/737688113

You other points are valid for me, I suffer the missing back button as well. Unfortunately this does not outweigh the usability shortcomings here.

@daschuer
Copy link
Member Author

@Holzhaus
Copy link
Member

But this is still no solution for the phone, where it switches back to desktop view.

The Full screen button works on my phone.

Did you see a mobile view? I can also switch to full screen but the text is not readable without a magnifier glass. I use Chrome on Android.

Ah, I didn't notice how small it is because I usually turn my phone sideways to read code. This is how it looks for me:

Screenshot_20201021_153546_com duckduckgo mobile android

@daschuer
Copy link
Member Author

I see a scrollbar on the right of the log box. Can't you just scroll down?

Unfortunately only the 6 lines are scrolled. The whole website is not scroll-able.

@daschuer
Copy link
Member Author

Idea: I could also look into checking the output ...

If you like to go this extra mile?
In the mean time, I prefer to keep it simple and just go back to Travis.

@uklotzde
Copy link
Contributor

I disagree. The full-screen view works well, why should we go back to Travis?

The pre-commit hook is supposed to be executed locally by developers before publishing a PR. The GitHub Action is only the final quality gate and not intended as a development tool.

@daschuer
Copy link
Member Author

I disagree. The full-screen view works well, why should we go back to Travis?

What environment do you use?

The issue is that new contributors will likely have the pre-commit hook not installed. This is the only case where the hook ever fails. For these users and for the reviewers we nee a smooth experience.

I think I have reasonable outlined that the full-screen view does not work well and is hard to discover. This applies especially to new contributors.

I will ask on Zulip.

@uklotzde uklotzde marked this pull request as draft October 29, 2020 22:11
@uklotzde
Copy link
Contributor

Converted to draft, because we don't have an agreement yet.

@daschuer
Copy link
Member Author

I will close this sind my opinion seems to be a minority.

@daschuer daschuer closed this Oct 29, 2020
@daschuer daschuer deleted the pre-commit-travis branch May 4, 2022 10:15
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