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

feat: Replace underscores with hyphens in project-name, fixes #6206 #6210

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mbomb007
Copy link
Contributor

@mbomb007 mbomb007 commented May 17, 2024

The Issue

Drupal modules frequently contain underscores in the project names. It's annoying to have to specify a different project name, or manually convert underscores to hyphens, each time I create a DDEV project for one. It seems a bit arbitrary that underscores aren't allowed.

How This PR Solves The Issue

When getting the project name from the directory name, underscores are replaced with hyphens.

Manual Testing Instructions

Automated Testing Overview

This needs an additional test to ensure that a project directory containing underscores now has the default project-name working.

Related Issue Link(s)

#6206

Release/Deployment Notes

@mbomb007 mbomb007 requested a review from a team as a code owner May 17, 2024 19:36
@mbomb007
Copy link
Contributor Author

@rfay I'm not sure how to create a test for a working directory containing underscores. I'm also not sure if this is the correct approach to a solution, but I figured that normalizing the name in as few locations as possible would be best. Passing names containing underscores for --project-name is also not something I decided to support.

Copy link

github-actions bot commented May 17, 2024

Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

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

This is looking great, thanks! Do you want to try creating the relevant test? It will be fun!

@mbomb007 mbomb007 requested a review from rfay May 21, 2024 20:32
@mbomb007
Copy link
Contributor Author

@rfay I added a test that creates a docroot with underscores, then uses the default suggested --project-name in the CLI.

@mbomb007
Copy link
Contributor Author

@rfay I updated the branch with the most recent changes, and suddenly more stuff is broken. I don't understand it, so you can take it from here.

@rfay
Copy link
Member

rfay commented May 21, 2024

Here's a situation where your code couldn't compile locally, I don't think, so where manual testing is so critical. I'm pretty sure make didn't work here.

It's also a place where you want to use the rebase workflow instead of merging master in, which hides so many things.

I'm quite sure that if you look at this you'll understand what happened to input, which is no longer defined.

@mbomb007 mbomb007 force-pushed the 20240517_mbomb007_normalize_sitename branch from 86fc29f to 71ec9f3 Compare May 22, 2024 14:08
@mbomb007
Copy link
Contributor Author

@rfay I got the checks to pass with a new test.

@rfay
Copy link
Member

rfay commented May 22, 2024

Congrats!

@rfay
Copy link
Member

rfay commented May 28, 2024

Would you mind adding a commit to this making the project name lower-case as well? docker-compose needs that.

We also remove dots/periods for docker-compose project name, but I don't think people would like removing those from the project name.

@mbomb007
Copy link
Contributor Author

@rfay Sure. If you really want that part covered by the tests as well, we can change one of the names in the test matrix to have a capital letter, so that the folder has a capital letter.

@rfay
Copy link
Member

rfay commented May 28, 2024

Improving tests would be awesome.

@rfay rfay changed the title feat: Replace underscores with hyphens for default --project-name, fixes #6206 feat: Replace underscores with hyphens and use lower-case for project-name, fixes #6206 May 28, 2024
cmd/ddev/cmd/config.go Outdated Show resolved Hide resolved
@rfay
Copy link
Member

rfay commented May 31, 2024

@mbomb007 somewhere you made a mess, I think by using the "merge" option on the github UI instead of rebase.

Anyway, there are an enormous number of conflicts here now. I started to rebase it, but I'm hoping you remember what you were trying to do and you can successfully rebase it.

I don't know if the default "update branch" (does a merge) will get you where you want to go now, but I'm certainly hesitant. Please do what you need to do, try "update branch" with merge commit if you want, but make sure you're getting the results you were aiming for.

@rfay
Copy link
Member

rfay commented Jun 2, 2024

We can also fix this by simply rebasing it into a single commit and we'll get it fixed that way.

@mbomb007
Copy link
Contributor Author

mbomb007 commented Jun 3, 2024

I don't have any knowledge of how to update my fork aside from clicking the button in the UI. I also have no relevant experience with rebasing.

@rfay
Copy link
Member

rfay commented Jun 3, 2024

I'll fix it.

@stasadev
Copy link
Member

stasadev commented Jun 3, 2024

I am wondering what is the best way to squash all the commits here into one. My strategy would be to do a soft reset to the point where this branch started, commit, rebase on master and resolve the conflicts.

BTW, the tests fail on TestRouterPortsCheck because of strings.ToLower() in NormalizeProjectName().
https://buildkite.com/ddev/macos-lima/builds/713#018fdee8-586b-4ceb-bb52-19e219b3c5a2/4678-4739

@rfay
Copy link
Member

rfay commented Jun 3, 2024

Yes, I think just squashing and then fixing. I think their merge did update it, but merge commits are pretty squirrely. But... Github offers them as the default!

@rfay rfay force-pushed the 20240517_mbomb007_normalize_sitename branch from d919798 to 4e7c633 Compare June 6, 2024 01:43
@rfay
Copy link
Member

rfay commented Jun 6, 2024

I squashed this into one commit, but then reverted the ToLower() as it had unknown consequences.

@mbomb007 mbomb007 force-pushed the 20240517_mbomb007_normalize_sitename branch from 4e7c633 to e567fd3 Compare June 6, 2024 14:07
@rfay rfay changed the title feat: Replace underscores with hyphens and use lower-case for project-name, fixes #6206 feat: Replace underscores with hyphens in project-name, fixes #6206 Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants