search

TrackableEntities / EntityFrameworkCore.Scaffolding.Handlebars

Scaffold EF Core models using Handlebars templates.
MIT License
210 stars 53 forks source link

[Deprecated] Pluralization implementation using Humanizer #109

Closed codingdna2 closed 4 years ago

codingdna2 commented 4 years ago

This PR includes previous modifications proposed in PR #108, adding a Pluralization service implemented with Humanizer package.

codingdna2 commented 4 years ago

A side note. I've included a reference to Humanizer.Core which includes only the English localization. I suppose it's enough for most scenarios. You can decide to include Humanizer package instead, which includes all the languages availables (but I've no idea if all of them implements pluralization).

Best Regards, D.

tonysneed commented 4 years ago

@ErikEJ Thanks for reviewing!

@codingdna2 Couple of questions / requests:

codingdna2 commented 4 years ago
  • What happens if non-English language is specified but not supported by Humanizer.Core? Will it revert to strings instead of nameof?

Two questions here:

  1. In case word is not recognized I'm not sure of the behavior, I guess it can happen two things, it gets confused or leave it like it is. There's no language detection, it's just selection of the right vocabulary. I've looked at the documentation but it's not clear and code is quite wide to dive into. I'll have a look again.
  2. If there are no problem in generating nameof (no conflicts, no compiler error in sight), it'll still use nameof.
  • Would you be able to separate commits for this PR from your other PR? The easiest is to use Visual Studio.

I guess is not a problem. Be aware that I'll need to remove the usage of nameof from ExpectedXXX (leaving, if you merge both PRs, with non-runinng tests).

  • Thanks for updating the ExpectedXXX classes.

You're welcome!

Would you be able to add a unit test for the pluralization option?

The unit tests are already running with pluralization by default (all the collections are plural). I see it requires quite a lot of effort to split them all to have both singular and plural tests. I need more time.

All in all, it would be easier if you can accept the first PR shortly (as is bugfix), so I can do a merge on the second PR with calm (as is enhancement). But I'll do what you prefer ;)

Regards, D.

tonysneed commented 4 years ago

I’ll be glad to merge #108 first. The problem is that you did not create a separate branch for the PR. In this case I advise you rename your fork, then refork my repo, so that your master branch matches mine.

For some guidance on creating a PR, please refer to the Contributing Guidelines.

codingdna2 commented 4 years ago

Ahahah I didn't notice up now. I guess this is what happens when you work from home while looking at your 4 years old daughter (or late night while she's sleeping)... I'll fix everything asap.

tonysneed commented 4 years ago

@codingdna2 Did you rename your fork, then refork my repo? That should give you the cleanest starting point.

codingdna2 commented 4 years ago

I've practiced and failed! I moved my modifications to @fix/pluralization but the merge complicated the squash... I guess I'm going to refork and move modifications manually.

tonysneed commented 4 years ago

@codingdna2 When you re-fork, remember to create a separate branch, according to the Contributing Guidelines.

codingdna2 commented 4 years ago

Almost completed including the separated tests.