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

ci: update symlink ci, extend tested cases #21466

Merged
merged 2 commits into from
May 8, 2024

Conversation

ttytm
Copy link
Member

@ttytm ttytm commented May 8, 2024

Regarding: 6cda618

  • Extends tested cases with less occupied runners.
  • Second is an incremental refactor to the related file and only concerns updating the workflow name to what is most consistently used for other workflows.

Comment on lines +47 to +50
- name: Unlink
run: |
rm $(which v)
v --version && exit 1 || exit 0
Copy link
Member Author

@ttytm ttytm May 8, 2024

Choose a reason for hiding this comment

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

Ref. ensuring that unlinking worked.
Trying to execute v --version while still linked (before removing the link).

Screenshot_20240508_083530

@spytheman
Copy link
Member

The changes seem good to me, but please do not rename files, in the same PR that changes their contents a lot. That needlessly necessitates checking out the changes locally.

@spytheman spytheman merged commit 3158f58 into vlang:master May 8, 2024
9 checks passed
@ttytm ttytm deleted the ci/update-symlink branch May 8, 2024 07:14
@ttytm
Copy link
Member Author

ttytm commented May 8, 2024

K understood.
Reason was that I thought some changes might be too pedantic or have trouble getting through on their own, so sneaking them in as an "incremental refactor" could be a way. I will choose them better.

@spytheman
Copy link
Member

I usually do not care much about the individual commits in a PR (since they often contain trivial stuff like fixing typos, quoting styles, word wrapping etc, to accommodate the different CI jobs and linters).

It frequently saves time, from my perspective, when possible, to review only the final diff/result, that is going to be merged, using Github's "Files changed" pane, after the CI passes, and a PR is no longer a draft.

The negative of that, is that my feedback and reviews are often delayed, by the time the CI finishes running (which is now minimized, in many cases, thanks to your efforts), and I may be an ass, from the perspective of contributors, who did care a lot about the individual commits.

Putting functional changes in PRs, that at the same time mention the word 'refactor', and that have file renames, is definitely something to be avoided.

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.

None yet

2 participants