Skip to content

Fix forgotten merge #2618

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 22, 2024
Merged

Fix forgotten merge #2618

merged 3 commits into from
Jul 22, 2024

Conversation

vmcj
Copy link
Member

@vmcj vmcj commented Jul 12, 2024

No description provided.

@vmcj vmcj mentioned this pull request Jul 12, 2024
Closed
tom93
tom93 reviewed Jul 13, 2024
View reviewed changes
tom93
tom93 reviewed Jul 13, 2024
View reviewed changes
Copy link
Contributor

@tom93 tom93 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

There are a couple of places that still mention lib/vendor, do they need updating?

Makefile:292:  -rm -rf lib/vendor/*
.github/jobs/data/codespellignorefiles.txt:13:./lib/vendor
.github/jobs/syntax-check:26:       -path ./lib/vendor        -prune \
.github/jobs/syntax-check:44:          echo "$i" | grep -qvE ...

Edit: Oops, I missed .github/jobs/syntax-check:51. I'll fix it in #2635.

@vmcj vmcj force-pushed the fix_forgotten_merge branch from 46ff14b to aaf78ff Compare July 13, 2024 08:51
tom93
tom93 reviewed Jul 15, 2024
View reviewed changes
@vmcj vmcj marked this pull request as ready for review July 20, 2024 10:30
@vmcj vmcj requested a review from a team July 20, 2024 10:36
tom93
tom93 reviewed Jul 20, 2024
View reviewed changes
Copy link
Contributor

@tom93 tom93 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Earlier commits introduced an issue, make maintainer-install doesn't actually run composer-dump-autoload-dev. I committed a fix and some other cleanup in branch 'fix_forgotten_merge_suggestions' of my fork (commits).

@vmcj
Copy link
Member Author

vmcj commented Jul 20, 2024

Earlier commits introduced an issue, make maintainer-install doesn't actually run composer-dump-autoload-dev. I committed a fix and some other cleanup in branch 'fix_forgotten_merge_suggestions' of my fork (commits).

I'll cherry-pick (& alter) some of those.

@vmcj vmcj force-pushed the fix_forgotten_merge branch 2 times, most recently from cd96088 to 12434f6 Compare July 21, 2024 15:54
@tom93
Copy link
Contributor

tom93 commented Jul 22, 2024 •
edited
Loading

@vmcj, what do you think about a0fd9fd? (That's the simplified relative paths -- ../webapp/vendor becomes vendor). Should I create a separate PR, or just drop it?

@vmcj
Copy link
Member Author

vmcj commented Jul 22, 2024

@vmcj, what do you think about a0fd9fd? (That's the simplified relative paths -- ../webapp/vendor becomes vendor). Should I create a separate PR, or just drop it?

I liked both of the commits but wanted to close this PR first.

So bugfix before refactoring.

vmcj and others added 3 commits July 22, 2024 16:02
@vmcj
Fix merge of last 2 commits
a6813a3
@vmcj
Revert indentation
bbdc99d 
It was added to fix the syntax highlighting but this also added this to
the commands. Picked from a commit from @tom93.
@tom93 @vmcj
Fix maintainer-install
b713d5d 
It wasn't running `composer-dump-autoload-dev` (the prerequisite in
the root Makefile doesn't work -- we need to recurse into ./webapp).

Rewritten by: @vmcj
@vmcj vmcj force-pushed the fix_forgotten_merge branch from 12434f6 to b713d5d Compare July 22, 2024 14:02
@vmcj vmcj added this pull request to the merge queue Jul 22, 2024
Merged via the queue into DOMjudge:main with commit 4fb25aa Jul 22, 2024
23 of 24 checks passed
@vmcj vmcj deleted the fix_forgotten_merge branch July 22, 2024 14:02
@tom93 tom93 mentioned this pull request Jul 29, 2024
Merged
@tom93
Copy link
Contributor

tom93 commented Jul 29, 2024

I liked both of the commits but wanted to close this PR first.

I created a separate PR (#2635).

@tom93 tom93 mentioned this pull request Jul 29, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nickygerritsen nickygerritsen nickygerritsen approved these changes

+1 more reviewer

@tom93 tom93 tom93 left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vmcj @tom93 @nickygerritsen