lando / backdrop

The Official Backdrop Lando Plugin
https://docs.lando.dev/backdrop/
GNU General Public License v3.0
1 stars 5 forks source link

Update mariadb plugin #47

Closed uberhacker closed 5 months ago

uberhacker commented 5 months ago

Bare minimum self-checks

What do you think of a person who only does the bare minimum?

Pieces of flare

Finally

If you have any issues or need help please join the #contributors channel in the Lando slack and someone will gladly help you out!

You can also check out the coder guide.

netlify[bot] commented 5 months ago

Deploy Preview for lando-backdrop ready!

Name Link
Latest commit df8bb3d5c3c11495d3677bfe75e8f4bf436d7e9c
Latest deploy log https://app.netlify.com/sites/lando-backdrop/deploys/663298610bc5820007cb6505
Deploy Preview https://deploy-preview-47--lando-backdrop.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

Lighthouse
1 paths audited
Performance: 94 (no change from production)
Accessibility: 96 (no change from production)
Best Practices: 100 (no change from production)
SEO: 93 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

uberhacker commented 5 months ago

Not a maintainer, but I'd recommend to make separate PRs for the mariadb update and the text changes, rather than combining changes into one.

Text cleanup is helpful but mixing changes in PRs can hide unexpected change.

Same thoughts as last review lando/acquia#95 (comment) on including version number and release date in CHANGELOG.

So are you suggesting the version should be 1.3.1 instead of 1.4.0? #51 mentions to update the docs. I figured a codespell check wouldn't hurt to clean up.

xurizaemon commented 5 months ago

So are you suggesting the version should be 1.3.1 instead of 1.4.0? https://github.com/lando/mariadb/issues/51 mentions to update the docs. I figured a codespell check wouldn't hurt to clean up.

More "I don't know, so I recommend to check what the correct next version and publication date will be before making that assertion" than "I think it should be different". Maybe that happens in the PR discussion 😄

Comparing https://github.com/lando/drupal/blob/main/CHANGELOG.md I suspect your version change to 1.4.0 is the correct one.

Personally, I would probably leave the date out of the CHANGELOG because I wouldn't want to guess at the release date. (For me it's easier to keep an "Unreleased" section at the top of the CHANGELOG, and let the maintainer make the updates when releases are cut, but that varies per project ... if Lando style is for the proposer to add a date, so be it!). See https://github.com/lando/backdrop/pull/47#discussion_r1583588239

Re "x says to update the docs" - agree, but update the docs within the scope of the proposed changes IMO. For general tidy up changes, a separate "Docs improvements" PR is what I'd aim for.

Hope this helps!

reynoldsalec commented 5 months ago

@uberhacker I think the error on the mariadb test is from using the mysql binary...I guess we'd have to add mariadb as a tooling command (similar to how we add mysql: https://github.com/lando/backdrop/blob/main/utils/get-tooling-defaults.js#L45

uberhacker commented 5 months ago

@uberhacker I think the error on the mariadb test is from using the mysql binary...I guess we'd have to add mariadb as a tooling command (similar to how we add mysql: https://github.com/lando/backdrop/blob/main/utils/get-tooling-defaults.js#L45

This should be ready to go now @reynoldsalec. All tests passed.