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.

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

Fix namespaced generators #1135

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@tyabe
Contributor

tyabe commented Mar 18, 2013

modified to handle namespace of Module is Project Name. and the Class is
Application Name.

I think this would be reasonable,
because The Admin App has same so structure.

Fix namespaced generators
modified to handle namespace of Module is Project Name. and the Class is
Application Name.
@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE

DAddYE Mar 18, 2013

Member

👍 by me, @achiu @skade ?

Member

DAddYE commented Mar 18, 2013

👍 by me, @achiu @skade ?

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 18, 2013

Member

For reference, there was a discussion about this in #1007, #741 and #1011. I am not opposed to that (as you can see, I suggested something similar), but finding a different way of detecting the project might be worthwhile (gem name?).

Member

skade commented Mar 18, 2013

For reference, there was a discussion about this in #1007, #741 and #1011. I am not opposed to that (as you can see, I suggested something similar), but finding a different way of detecting the project might be worthwhile (gem name?).

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 19, 2013

Contributor

I tried to the same way as the Application Name to get the Project Name.
How does that sound?

Contributor

tyabe commented Mar 19, 2013

I tried to the same way as the Application Name to get the Project Name.
How does that sound?

@DAddYE

This comment has been minimized.

Show comment
Hide comment
@DAddYE
Member

DAddYE commented Mar 20, 2013

@skade ?

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

I think this issue want to solve before 0.11.0 release, what do you guys think @padrino/core-members ?

Contributor

tyabe commented Mar 21, 2013

I think this issue want to solve before 0.11.0 release, what do you guys think @padrino/core-members ?

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 21, 2013

Member

@padrino/core-members should I merge this before the release? Looks good to me?

Member

nesquena commented Mar 21, 2013

@padrino/core-members should I merge this before the release? Looks good to me?

@nesquena nesquena referenced this pull request Mar 21, 2013

Closed

Release 0.11.0 #755

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

I am still not very happy with the lookup method, but am out of good suggestions :/.

Member

skade commented Mar 21, 2013

I am still not very happy with the lookup method, but am out of good suggestions :/.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

The problem now is that we rely on the presence of a folder called "app" if I see correctly. Thats not the case in all applications.

Member

skade commented Mar 21, 2013

The problem now is that we rely on the presence of a folder called "app" if I see correctly. Thats not the case in all applications.

@nesquena

This comment has been minimized.

Show comment
Hide comment
@nesquena

nesquena Mar 21, 2013

Member

I see that's a good point, hmm I wonder what could be a more robust way to determine the names...

Member

nesquena commented Mar 21, 2013

I see that's a good point, hmm I wonder what could be a more robust way to determine the names...

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

I'd really like to have that in :(. In Gems, this is actually rather trivial, as the main module is a Padrino::Module and registered. So maybe going for more symmetry there would help things.

Member

skade commented Mar 21, 2013

I'd really like to have that in :(. In Gems, this is actually rather trivial, as the main module is a Padrino::Module and registered. So maybe going for more symmetry there would help things.

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

I fully agree to @skade. However, I cannot find better than way now for the Sub App :(

Contributor

tyabe commented Mar 21, 2013

I fully agree to @skade. However, I cannot find better than way now for the Sub App :(

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

Okay, did a quick user survey. First of all: why not save the project namespace in .components?

Also, the ability to pick the namespace per app at generation time would be nice. e.g.:

padrino gen app uploader --namespace "admin"

Member

skade commented Mar 21, 2013

Okay, did a quick user survey. First of all: why not save the project namespace in .components?

Also, the ability to pick the namespace per app at generation time would be nice. e.g.:

padrino gen app uploader --namespace "admin"

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

Awesome! 👏

Contributor

tyabe commented Mar 21, 2013

Awesome! 👏

@dariocravero

This comment has been minimized.

Show comment
Hide comment
@dariocravero

dariocravero Mar 21, 2013

Contributor

👍!!
On Mar 21, 2013 8:04 AM, "Takeshi Yabe" notifications@github.com wrote:

Awesome! [image: 👏]


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1135#issuecomment-15230569
.

Contributor

dariocravero commented Mar 21, 2013

👍!!
On Mar 21, 2013 8:04 AM, "Takeshi Yabe" notifications@github.com wrote:

Awesome! [image: 👏]


Reply to this email directly or view it on GitHubhttps://github.com/padrino/padrino-framework/pull/1135#issuecomment-15230569
.

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

I tried implement. test is not enough, but existing test is all green :)

Contributor

tyabe commented Mar 21, 2013

I tried implement. test is not enough, but existing test is all green :)

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

The implementation looks good, safe for one detail: what about old apps that don't have the choice stored yet?

Sorry, feeling a bit like Statler and Waldorf today: all complaining, no doing ;).

Statler and Waldorf

Member

skade commented Mar 21, 2013

The implementation looks good, safe for one detail: what about old apps that don't have the choice stored yet?

Sorry, feeling a bit like Statler and Waldorf today: all complaining, no doing ;).

Statler and Waldorf

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

Okey, old apps pick the namespace from Module Name, Or --namespace options.

Contributor

tyabe commented Mar 21, 2013

Okey, old apps pick the namespace from Module Name, Or --namespace options.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

This breaks. It tries to detect the app name from a file that is yet to be written ;).

[ skade gemified ] bundle exec padrino gen app foo_bar
/Users/skade/padrino/padrino-framework/padrino-framework/padrino-gen/lib/padrino-gen/generators/actions.rb:244:in `read': No such file or directory - /Users/skade/padrino/padrino-framework/padrino-framework/gemified/foo_bar/app.rb (Errno::ENOENT)
    from /Users/skade/padrino/padrino-framework/padrino-framework/padrino-gen/lib/padrino-gen/generators/actions.rb:244:in `fetch_project_name'

I think as a fallback, the folder name is really the better guess. I created a fix here:

skade@ef9fd7b

Could you please cherry-pick that on top? Then I'll merge.

Member

skade commented Mar 21, 2013

This breaks. It tries to detect the app name from a file that is yet to be written ;).

[ skade gemified ] bundle exec padrino gen app foo_bar
/Users/skade/padrino/padrino-framework/padrino-framework/padrino-gen/lib/padrino-gen/generators/actions.rb:244:in `read': No such file or directory - /Users/skade/padrino/padrino-framework/padrino-framework/gemified/foo_bar/app.rb (Errno::ENOENT)
    from /Users/skade/padrino/padrino-framework/padrino-framework/padrino-gen/lib/padrino-gen/generators/actions.rb:244:in `fetch_project_name'

I think as a fallback, the folder name is really the better guess. I created a fix here:

skade@ef9fd7b

Could you please cherry-pick that on top? Then I'll merge.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

What am I talking, I'll merge this myself.

Member

skade commented Mar 21, 2013

What am I talking, I'll merge this myself.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

Rebased and merged. Thank you for reporting and discussing issue as well as building the solution, @tyabe. You helped us alot.

Member

skade commented Mar 21, 2013

Rebased and merged. Thank you for reporting and discussing issue as well as building the solution, @tyabe. You helped us alot.

@skade

This comment has been minimized.

Show comment
Hide comment
@skade

skade Mar 21, 2013

Member

Closed, now to be found in 2cdce13.

Member

skade commented Mar 21, 2013

Closed, now to be found in 2cdce13.

@skade skade closed this Mar 21, 2013

@tyabe

This comment has been minimized.

Show comment
Hide comment
@tyabe

tyabe Mar 21, 2013

Contributor

👌 Thanks! @skade

Contributor

tyabe commented Mar 21, 2013

👌 Thanks! @skade

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment