refactor: localize crud templates #172

namit-chandwani · 2021-07-23T18:21:31Z

Closes #175

Description

Add a CRUD locale file template for generation (along with the CRUD command files).

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation change
Other (please specify): Refactor

Checklist

Documentation added for the feature
CI and all relevant tests are passing
Code Review completed
Verified independently by reviewer

namit-chandwani · 2021-07-23T18:25:22Z

pkg/cmd/crud/crud.go

@@ -109,6 +111,11 @@ func generateCrudFiles() error {
 // generateFileFromTemplate uses the passed contents and data object of a
 // template to generate a new file using the specified file name and output path
 func generateFileFromTemplate(name, path, tmplContent string, tmplData interface{}) error {
+	// Sets appropriate target path for the locale file
+	if name == "crud.en.yaml" {
+		path = "./cmd/abc/locales/en"


I'd prefer this PR to be not merged right now, since the target path for the generated locale file has been hardcoded for now.
Once the charmil init command has been completely set up, the following change can be made to make the path dynamic:

Suggested change

path = "./cmd/abc/locales/en"

path = "./cmd/[NAME_OF_CLI]/locales/en"

charmil init is working, just a small issue with .git, not working for already initialized git folders.
I have used a templateContext struct in which I have asked github owner, repo, cliname. So I think we can save cliname somewhere to take reference.

you can walk the directory, cmd/CLI_NAME. So you can get the cliname from here right?

charmil init is working, just a small issue with .git, not working for already initialized git folders.

Yes, that's not the issue.
I was hoping we could save the CLI metadata from charmil init somewhere.

I have used a templateContext struct in which I have asked github owner, repo, cliname. So I think we can save cliname somewhere to take reference.

This is exactly what I was talking about. I'll have a look at the templateContext.

Thanks!

namit-chandwani

I feel the PR will be only ready to merge after the change highlighted above is made and for that we will need to integrate this with the charmil init command

ankithans · 2021-07-23T18:35:17Z

i am not really sure, if the commands can be integrated
copying the same line as above - this might be the solution

you can walk the directory, cmd/CLI_NAME. So you can get the cliname from here right?

namit-chandwani · 2021-07-23T18:43:57Z

you can walk the directory, cmd/CLI_NAME. So you can get the cliname from here right?

Yes, but that can only be done if the CLI is initialized using charmil init, right?
Because if we allow the developers use their own CLI, the directory structure might be different hence this approach might not work.

(Ref: #67 (comment))

I have used a templateContext struct in which I have asked github owner, repo, cliname. So I think we can save cliname somewhere to take reference.

Yeah, this is what I prefer too (as mentioned here: #172 (comment)).

But I think we will have to finalize what we want to do with this discussion, before proceeding with this PR.

ankithans · 2021-07-23T18:57:51Z

I have used a templateContext struct in which I have asked github owner, repo, cliname. So I think we can save cliname somewhere to take reference.

Yeah, this is what I prefer too

I can make a file with these fields it will be easy, but we must make sure if that is not unnecessary. We should first think of any way of doing without doing above. Like one way is walking through dir. Other may be leave some signatures of charmil init in files, that this code is generated from charmil init command.

And charmil crud should work for the users who haven't used charmil init command. We don't want to enforce this on them ig.
I know it will require config, factory, locales from the root command, this is the issue

namit-chandwani · 2021-07-23T19:08:26Z

I know it will require config, factory, locales from the root command, this is the issue

Exactly, I believe if we plan to use some core features like config later, allowing developers to use only the CLI generated using charmil init would be much better.

I can make a file with these fields it will be easy, but we must make sure if that is not unnecessary. We should first think of any way of doing without doing above. Like one way is walking through dir. Other may be leave some signatures of charmil init in files, that this code is generated from charmil init command.

And charmil crud should work for the users who haven't used charmil init command. We don't want to enforce this on them ig.

Also, the solutions that you have proposed above might not work if we allow the developers to use their own CLI, since the project structure etc., can be different.

So that gives us another reason to restrict developers to using charmil init before charmil crud

ankithans · 2021-07-23T19:10:51Z

pinging @wtrocki
TLDR of above

Exactly, I believe if we plan to use some core features like config later, allowing developers to use only the CLI generated using charmil init would be much better.

wtrocki · 2021-07-23T20:25:50Z

Without taking any sides here I see two approqches drafted.

Highly integrated
Just generate some files for me please and let dev do piping

My take is to just dump some files into two folders with crud command and let users to connect them with cli aka take 2, but I might be wrong.

Core will be automatically imported when running go mod tidy so I do not see much issue of knowing where user generates them. We could easily generate those in non charmil starter and get it to work.

I could be completely wrong here but my first impression is that generator should be dam simple and small.

EDIT:

So that gives us another reason to restrict developers to using charmil init before charmil crud

I must clearly miss some context because this restriction doesn't occur to me as needed until we mentioned it.
Can you point out what is the reason for it.

wtrocki · 2021-07-27T10:13:51Z

@namit-chandwani can you take look on this and push all fixes so we could integrate it. I would love to test CRUD features as as soon as possible and this PR is blocking me right now

namit-chandwani · 2021-07-27T10:17:00Z

@namit-chandwani can you take look on this and push all fixes so we could integrate it. I would love to test CRUD features as as soon as possible and this PR is blocking me right now

On it

namit-chandwani · 2021-07-27T10:48:06Z

@aerogear/charmil This is now ready for review :)

wtrocki · 2021-07-27T11:00:55Z

Rebase needed. @ankithans can you review

ankithans

minor changes -
creates a crud folder if this err occurs, means empty folder
lstat ./cmd: no such file or directory

then i try it gives
mkdir ./crud: file exists

I think default path for crud files can be pkg/cmd/ and if this path doesn't exist then root dir

Rest it works great 👍

wtrocki · 2021-07-27T11:17:16Z

New release after merge please

namit-chandwani · 2021-07-27T15:29:11Z

minor changes -
creates a crud folder if this err occurs, means empty folder
lstat ./cmd: no such file or directory

then i try it gives
mkdir ./crud: file exists

@ankithans Fixed these errors. Please try running now :)

ankithans · 2021-07-27T15:42:30Z

yeah it works.. great
what's your take on the default folder? Should'nt it be ./pkg/cmd? and if this doesn't exist -> ./

wtrocki · 2021-07-27T16:23:27Z

current folder is best

refactor: localize crud templates

5cc19e8

namit-chandwani commented Jul 23, 2021

View reviewed changes

ankithans added this to the Charmil CLI - shell commands milestone Jul 25, 2021

refactor: make locale path dynamic

8fa2e63

namit-chandwani marked this pull request as ready for review July 27, 2021 10:41

namit-chandwani requested review from wtrocki and ankithans July 27, 2021 10:41

ankithans reviewed Jul 27, 2021

View reviewed changes

namit-chandwani added 5 commits July 27, 2021 18:56

chore: add flag entries to crud locale file

a9c0bc2

fix: resolve dir already exists error

b138be2

fix: delete the broken crud dir if error occurs

784544d

docs: update inline docs

b71deb4

fix: resolve build issues

13ac85e

namit-chandwani requested a review from ankithans July 27, 2021 15:29

ankithans approved these changes Jul 27, 2021

View reviewed changes

wtrocki merged commit 020b6c4 into main Jul 27, 2021

wtrocki deleted the refactor/template/localize-crud-templates branch July 27, 2021 16:23

namit-chandwani mentioned this pull request Jul 27, 2021

feat: add flags to charmil crud commands #186

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: localize crud templates #172

refactor: localize crud templates #172

namit-chandwani commented Jul 23, 2021 •

edited

namit-chandwani Jul 23, 2021 •

edited

ankithans Jul 23, 2021 •

edited

namit-chandwani Jul 23, 2021

namit-chandwani left a comment •

edited

ankithans commented Jul 23, 2021

namit-chandwani commented Jul 23, 2021 •

edited

ankithans commented Jul 23, 2021 •

edited

namit-chandwani commented Jul 23, 2021 •

edited

ankithans commented Jul 23, 2021

wtrocki commented Jul 23, 2021 •

edited

wtrocki commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021

wtrocki commented Jul 27, 2021

ankithans left a comment

wtrocki commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021 •

edited

ankithans commented Jul 27, 2021

wtrocki commented Jul 27, 2021

	path = "./cmd/abc/locales/en"
	path = "./cmd/[NAME_OF_CLI]/locales/en"

refactor: localize crud templates #172

refactor: localize crud templates #172

Conversation

namit-chandwani commented Jul 23, 2021 • edited

Description

Type of change

Checklist

namit-chandwani Jul 23, 2021 • edited

Choose a reason for hiding this comment

ankithans Jul 23, 2021 • edited

Choose a reason for hiding this comment

namit-chandwani Jul 23, 2021

Choose a reason for hiding this comment

namit-chandwani left a comment • edited

Choose a reason for hiding this comment

ankithans commented Jul 23, 2021

namit-chandwani commented Jul 23, 2021 • edited

ankithans commented Jul 23, 2021 • edited

namit-chandwani commented Jul 23, 2021 • edited

ankithans commented Jul 23, 2021

wtrocki commented Jul 23, 2021 • edited

wtrocki commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021

wtrocki commented Jul 27, 2021

ankithans left a comment

Choose a reason for hiding this comment

wtrocki commented Jul 27, 2021

namit-chandwani commented Jul 27, 2021 • edited

ankithans commented Jul 27, 2021

wtrocki commented Jul 27, 2021

namit-chandwani commented Jul 23, 2021 •

edited

namit-chandwani Jul 23, 2021 •

edited

ankithans Jul 23, 2021 •

edited

namit-chandwani left a comment •

edited

namit-chandwani commented Jul 23, 2021 •

edited

ankithans commented Jul 23, 2021 •

edited

namit-chandwani commented Jul 23, 2021 •

edited

wtrocki commented Jul 23, 2021 •

edited

namit-chandwani commented Jul 27, 2021 •

edited