Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Improved generator using flect to handle plurals properly #1455

Merged
merged 2 commits into from
Nov 15, 2018

Conversation

sio4
Copy link
Member

@sio4 sio4 commented Nov 13, 2018

Improved generator to handle plurals properly, using flect instead of inflect.
It will solve issue #1440.

Please check the test case I fixed. I think it will work smoothly but I cannot sure since this is the first time I look into flect and this parts of codes.

Thanks.

@sio4 sio4 requested a review from a team as a code owner November 13, 2018 11:45
@sio4 sio4 changed the title improved generator using flect to handle plurals properly WIP: improved generator using flect to handle plurals properly Nov 13, 2018
@sio4
Copy link
Member Author

sio4 commented Nov 13, 2018

Sorry for annoying comments during explorering. :-) I edited comments and removed some useless comments.

Question 1

I found function File() of flect.name lately and it solve several problems. But still, URL() of flect is not fit for nested resource something like admin/planes. So I think the following patch for flect is required.

--- a/name/url.go
+++ b/name/url.go
@@ -1,5 +1,5 @@
 package name
 
 func (n Ident) URL() Ident {
-       return Ident{n.Pluralize().Underscore()}
+       return Ident{n.File().Pluralize()}
 }

How do you think about this? Can I PR it before further progress?

@sio4
Copy link
Member Author

sio4 commented Nov 13, 2018

Question 2

When I fall into the r.Auto(...), I have no idea to get paths of the templates since there is only model (name and pname) reflected from data. To get nested template paths, is there any good approach with the current structure?

Currently, I have two Ideas

  1. Add context value named "template" before call r.Auto() and use it when making template path.
  2. Just preserve the current structure and handle it in template side: do not use Auto() but use HTML() for nested resources.

Any Ideas?

@markbates
Copy link
Member

The results of flect should match the results of inflect, so if you have to make changes there, go for it!

Please don’t wait for permission to open a PR, I would rather make suggestion on a PR than for you to wait for me to answer a question. 🙂

The answer to question 2 is leave. It as is. I investigated “true” nested resources ages ago, but the Go package structure makes it difficult as well.

Another this is the generator will be the next to be converted to genny, once I get the PR with the other generators merged. Don’t spend too much time improving this generator. Any change you make now will need to be ported to the genny implementation.

@markbates markbates added this to the v0.14.0 milestone Nov 13, 2018
@markbates markbates added the s: in progress Someone is working on this label Nov 13, 2018
@sio4
Copy link
Member Author

sio4 commented Nov 14, 2018

Hi @markbates,

The results of flect should match the results of inflect, so if you have to make changes there, go for it!

Please don’t wait for permission to open a PR, I would rather make suggestion on a PR than for you to wait for me to answer a question.

OK, I opened PR for flect. Please check the PR. Since template feature uses the function directly, it is recommended that the PR need to be merged before this PR works properly.

The answer to question 2 is leave. It as is. I investigated “true” nested resources ages ago, but the Go package structure makes it difficult as well.

Another this is the generator will be the next to be converted to genny, once I get the PR with the other generators merged. Don’t spend too much time improving this generator. Any change you make now will need to be ported to the genny implementation.

OMG! yes, I knew about your recent works on another branch! but I forget that and I just ran into this issue when I show the issue was labeled as help wanted :-) Anyway, I added one more commit to this PR and I think this is not a bad approach to handle this issue (and to handle some miss structured template directories or when we want to reuse existing template for other resources, of course, this is a rare case but I think it can happen for simple/similar resources)

Anyway, It works now for potatoes! (including admin/super_heavy/potatoes)

Many thanks!

@sio4 sio4 changed the title WIP: improved generator using flect to handle plurals properly Improved generator using flect to handle plurals properly Nov 14, 2018
@markbates markbates removed the s: in progress Someone is working on this label Nov 15, 2018
@markbates markbates merged commit 7cc85d1 into gobuffalo:development Nov 15, 2018
@sio4 sio4 deleted the potatoes branch November 19, 2018 01:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants