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

Rename links-mysql8 driver to links-mysql #1189

Merged
merged 5 commits into from
Nov 15, 2023

Conversation

frank-emrich
Copy link
Contributor

This PR renames the links-mysql8 driver to links-mysql.

The idea is that for end users who upgrade from Links 0.9.7 to 0.9.8, the composition of #1178 (which removed the existing links-mysql driver) and this one has the effect of the links-mysql driver changing from using the mysql8 package under the hood in the future, with no further changes from users required.

@frank-emrich
Copy link
Contributor Author

This resolves #1182

@frank-emrich
Copy link
Contributor Author

Note that parts of the CI now state "Waiting for status to be reported". That's because I've renamed some of the CI steps, all tests are actually passing as intended. Once this PR lands I will perform the same renaming on which CI steps Github expects to have passed.

@dhil
Copy link
Member

dhil commented Oct 18, 2023

Note that parts of the CI now state "Waiting for status to be reported". That's because I've renamed some of the CI steps, all tests are actually passing as intended. Once this PR lands I will perform the same renaming on which CI steps Github expects to have passed.

Just delete the stale steps in the settings tab of this repository.

Copy link
Contributor

@jamescheney jamescheney left a comment

Choose a reason for hiding this comment

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

This looks fine generally, I have a couple of minor questions.

  1. Looking at tests/database, there are currently two (identical) files factorials.mysql8 and factorials.mysql, containing MySQL-specific SQL code to initialize the database for the test factorials.links. My guess is that after making this change, only the factorials.mysql file will ever be used by the test scripts, so the factorials.mysql8 file can be deleted.

  2. Looking at the changes to tests/relational-lenses, I was initially surprised to see that config.mysql8 is deleted and no config.mysql is created. The reason seems to be that config.mysql exists and has the correct contents already, presumably because it was not deleted earlier. So that's fine. However, looking at the rest of this directory, it appears that there are again identical files serial.mysql and serial.mysql8 containing MySQL-specific database setup code where the second one will be dead code after this cange and so can be deleted now.

I mark "request changes" now because these bits of cleanup should probably happen now.

@jamescheney jamescheney linked an issue Nov 14, 2023 that may be closed by this pull request
@frank-emrich
Copy link
Contributor Author

Thanks, those file were indeed meant to be deleted, good catch!

@frank-emrich
Copy link
Contributor Author

@jamescheney If you don't object I will merge this tomorrow.

@jamescheney jamescheney self-requested a review November 15, 2023 22:41
@jamescheney
Copy link
Contributor

Sorry, I didn't realize this was still waiting on me, LGTM.

@frank-emrich frank-emrich merged commit 0651c5f into links-lang:master Nov 15, 2023
9 checks passed
@frank-emrich frank-emrich deleted the mysql8-redux branch November 15, 2023 23:05
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.

Rename links-mysql8 driver to links-mysql?
3 participants