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.

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

Remove api folder. Add backend folder using latest backend best practices #6110

Merged
merged 6 commits into from
Jun 19, 2024
Merged

Remove api folder. Add backend folder using latest backend best practices #6110

merged 6 commits into from
Jun 19, 2024

Conversation

sneridagh
Copy link
Member

@sneridagh sneridagh commented Jun 17, 2024 •
edited by github-actions bot
Loading

📚 Documentation preview 📚: https://volto--6110.org.readthedocs.build/

@sneridagh sneridagh requested review from davisagli, ericof and a team June 17, 2024 17:00
@netlify Netlify
Copy link

netlify bot commented Jun 17, 2024 •
edited
Loading

✅ Deploy Preview for plone-components canceled.

Name Link
🔨 Latest commit d45014f
🔍 Latest deploy log https://app.netlify.com/sites/plone-components/deploys/66727c550eedf80008f85ecd

stevepiercy
stevepiercy requested changes Jun 18, 2024
View reviewed changes
backend/Makefile Outdated Show resolved Hide resolved
backend/Makefile Outdated Show resolved Hide resolved
backend/Makefile Outdated Show resolved Hide resolved
backend/Makefile Outdated Show resolved Hide resolved
backend/Makefile Outdated Show resolved Hide resolved
backend/README.md Outdated Show resolved Hide resolved
backend/setup.py Outdated Show resolved Hide resolved
backend/setup.py Outdated
@@ -0,0 +1,68 @@
"""Installer for the collective.addon package."""
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file shows its age from copy-pasta. We should port these fixes back into the source, whatever that is.

Also can we use cookiecutter to generate this?

backend/setup.py Outdated Show resolved Hide resolved
backend/setup.py Outdated Show resolved Hide resolved
@stevepiercy
Copy link
Collaborator

@terapyon @tisto can we remove the broken link to https://www.cmscom.jp/ from the README?

@tisto
Copy link
Member

tisto commented Jun 18, 2024

@stevepiercy I guess if that breaks our setup there is no alternative. Maybe we can comment it out for now, so that we can re-add it once @terapyon fixed the SSL issue?

@sneridagh
Copy link
Member Author

@stevepiercy This is the output of the cookieplone backend folder. We did already a review of it! If you catched new things we should get back there and fix them.

@stevepiercy
Copy link
Collaborator

@stevepiercy This is the output of the cookieplone backend folder. We did already a review of it! If you catched new things we should get back there and fix them.

OK, it smelled like boilerplate, I just didn't know from where. I'll update the templates.

One question, can we not use collective.addon as the package name when generating output? That threw me for a loop because of https://github.com/collective/collective.addons. Perhaps using the literal variable name when generating the output would be more clear that it is not intended for distribution as is?

@stevepiercy
Copy link
Collaborator

@tisto I've @-ed @terapyon in another PR last weekend, but he has not responded. I just sent him email. Let's give him a few days, and he doesn't respond in another week or so, we can remove it and let him know.

@stevepiercy
Copy link
Collaborator

PR opened plone/cookieplone-templates#41

@terapyon
Copy link
Member

I'm sorry too late response. I'll soon fix it.

@terapyon
Copy link
Member

I just fixed our site. I'm sorry too late.

@stevepiercy
Copy link
Collaborator

I just fixed our site. I'm sorry too late.

No worries. Thank you!

Hey, maybe we should charge for this monitoring service! 😁

ericof and others added 2 commits June 18, 2024 07:51
@ericof @stevepiercy
Apply suggestions from code review
30a8f9a 
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@sneridagh @stevepiercy
Apply suggestions from code review
aade85d 
Co-authored-by: Steve Piercy <web@stevepiercy.com>
@sneridagh
Copy link
Member Author

@stevepiercy back to you, I think it's ok.

davisagli
davisagli approved these changes Jun 19, 2024
View reviewed changes
Copy link
Member

@davisagli davisagli left a comment

Choose a reason for hiding this comment

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

Maybe I'm being stupid or stuck in a particular habit of working, but I don't really understand the motivation for including a backend project in the volto repository. In practice I never really use it.

If I want to run volto against the latest Plone release then I use make start-backend-docker. If I want to make a change to the Plone backend and test volto with it before it's merged, then I run the backend from the coredev buildout.

I guess this backend project gives people a way to install an editable copy of a backend package to try out a change in a way that's more similar to actual projects, without needing to learn buildout. That's probably a good thing.

Maybe there are other reasons to use it that I'm overlooking?

@sneridagh
Copy link
Member Author

@davisagli not really, it was always here, for convenience. Could be that nowadays is not worth the effort to have it anymore.

@plone/volto-team opinions?
+1 to remove it here.

@stevepiercy
Copy link
Collaborator

/api was not documented, at least not that I could find. For that reason alone, purge it.

Also because it is a snapshot generated from https://github.com/plone/cookieplone-templates/ with hardcoded values, I would rather use the cookiecutter to generate the latest version, using my own values, then move it in place.

Until mxdev is ready to take over, it's either buildout or Docker. Let's not create another officially supported method.

@ichim-david
Copy link
Member

I'm ok with removing as well although it's a bit sad to see work done only to realize that it's best to simply remove than merge the work :)

@sneridagh
Copy link
Member Author

sneridagh commented Jun 19, 2024 •
edited
Loading

Ok, removed! Wondering if we should remove Guillotina related stuff from the repo too. Opinions?

@sneridagh
Copy link
Member Author

ok, decided not to. Merging.

@sneridagh sneridagh merged commit 75403e3 into main Jun 19, 2024
70 checks passed
@sneridagh sneridagh deleted the convenience-backend-update branch June 19, 2024 08:12
sneridagh added a commit that referenced this pull request Jun 19, 2024
@sneridagh
Merge branch 'main' into renamemakefiletargets
fc6285a 
* main:
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
sneridagh added a commit that referenced this pull request Jun 19, 2024
@sneridagh
Merge branch 'main' into teaser-dynamic-again
f87d101 
* main:
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
sneridagh added a commit that referenced this pull request Jun 20, 2024
@sneridagh
Merge branch 'main' into fix-recursive-copy-container-blocks-on-copy
28deebd 
* main:
  Make dynamic/live teaser without additional requests (#6024)
  Fix issue of duplicated blocks upon pasting an image into the Slate E… (#5818)
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
sneridagh added a commit that referenced this pull request Jun 20, 2024
@sneridagh
Merge branch 'main' into fix_diff_non_contentroutes_path
bb99324 
* main:
  Make dynamic/live teaser without additional requests (#6024)
  Fix issue of duplicated blocks upon pasting an image into the Slate E… (#5818)
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
sneridagh added a commit that referenced this pull request Jun 26, 2024
@sneridagh
Merge branch 'main' into vitesupport
6205749 
* main: (73 commits)
  Release 18.0.0-alpha.36
  Rename missing command
  Image widget PR as breaking (#6125)
  Release @plone/slate 18.0.0-alpha.14
  Release @plone/registry 1.7.0
  Rename Makefile targets (#6104)
  fix: nonContentRoutes diff path (#6102)
  Automatically set the label to `03 type: feature (plip)` for PLIPs (#6122)
  Add ImageWidget with upload/drop/external and inline/widget capabilities (#5607)
  Ensure that sidebar field will not steal focus when metadata is edited (#5983)
  Prevent duplicated UUUIDs in inner blocks when copying container blocks (#6112)
  feat: handle breakList in detached TextBlockEditor (#6106)
  Make dynamic/live teaser without additional requests (#6024)
  Fix issue of duplicated blocks upon pasting an image into the Slate E… (#5818)
  Remove `api` folder. Add `backend` folder using latest backend best practices (#6110)
  Rename files with wrong extension `js->jsx` when they contain JSX. (#6114)
  Improve shadowing by including the support for `js->jsx` extensions in… (#6113)
  issue-5452 markdown shortcut (#5495)
  Release 18.0.0-alpha.35
  Release @plone/types 1.0.0-alpha.16
  ...
@stevepiercy stevepiercy mentioned this pull request Jul 9, 2024
Closed
@wesleybl wesleybl restored the convenience-backend-update branch July 15, 2024 14:05
@wesleybl wesleybl deleted the convenience-backend-update branch July 15, 2024 15:05
@davisagli davisagli mentioned this pull request Oct 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stevepiercy stevepiercy stevepiercy requested changes

@davisagli davisagli davisagli approved these changes

@ericof ericof Awaiting requested review from ericof

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@sneridagh @stevepiercy @tisto @terapyon @ichim-david @davisagli @ericof