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.

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

[MRG+1] Included new optional parameter in startproject command line #2005

Merged
merged 10 commits into from Jul 19, 2016

Conversation

@feliperuhland
Copy link
Contributor

@feliperuhland feliperuhland commented May 24, 2016

I don't know if anyone creates project like me.
I have a habit to create git repository and then I create scrapy project.

You can see something similar at django startproject command.

Unfortunately, I had to rewrite copytree function, to make sure that the startproject won't fail if it project_dir already exists.

I am available to any feedback,

Thanks.

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented May 24, 2016

Hello, @feliperuhland.

I like the feature idea, it will be nice to be able to do scrapy startproject myproj . inside a directory.

The build is failing for Python 3, can you have a look?

@feliperuhland
Copy link
Contributor Author

@feliperuhland feliperuhland commented May 24, 2016

Thanks, @eliasdorneles . I am working on it.

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented May 24, 2016

The template should also be updated to print a message with the correct project dir.

I just tried it with:

$ scrapy startproject myproj customdir

And it printed:

You can start your first spider with:
    cd myproj
    scrapy genspider example example.com

It should point me to cd customdir

@feliperuhland
Copy link
Contributor Author

@feliperuhland feliperuhland commented May 24, 2016

@eliasdorneles you're right.
I will fix this.

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented May 24, 2016

Looks great, thanks @feliperuhland !

@eliasdorneles eliasdorneles changed the title Included new optional parameter in startproject command line [MRG+1] Included new optional parameter in startproject command line May 24, 2016
@redapple redapple added this to the v1.2 milestone Jul 8, 2016

if len(args) == 2:
project_dir = args[1]
if exists(join(project_dir, 'scrapy.cfg')):

This comment has been minimized.

@eliasdorneles

eliasdorneles Jul 11, 2016
Member

hey @feliperuhland, I'm looking at this again, and I'm thinking this check if scrapy.cfg exists is needed in both cases (args == 1 or args == 2), right?
looks like it should be outside of this if.

This comment has been minimized.

@feliperuhland

feliperuhland Jul 13, 2016
Author Contributor

Not really, because in the case of len(args) == 1 the directory will be created and can't have the scrapy.cfg. But I can't see any problem in this change. What do you think? Thanks.

This comment has been minimized.

@eliasdorneles

eliasdorneles Jul 13, 2016
Member

Since we're using a custom copytree that now works in existing directories, I think we shouldn't assume the directory will be created.
Users can now do mkdir myproj; scrapy startproject myproj and scrapy will work without complaining, therefore the extra check is a good idea.

In fact, I just tried running scrapy startproject myproj twice and it breaks the second time.

elif _module_exists(project_name):
print('Error: Module %r already exists' % project_name)
else:
return True
return False

def _copytree(self, src, dst):

This comment has been minimized.

@eliasdorneles

eliasdorneles Jul 11, 2016
Member

Also, do you think we could get rid of this function, if we use the ignore callable argument for shutil.copytree ?

This comment has been minimized.

@feliperuhland

feliperuhland Jul 13, 2016
Author Contributor

Hi, @eliasdorneles. I would like to remove that function, but the original implementation always creates the destination directory and that is not useful to us. I'm not happy of rewrite some internal library behavior. Do you have any other ideias? Thanks again.

This comment has been minimized.

@eliasdorneles

eliasdorneles Jul 13, 2016
Member

Ah yeah, you're right.
The problem is that os.makedirs call that is always called and requires the directory not to exist.
Can you perhaps add a docstring explaining this, then?

@eliasdorneles eliasdorneles changed the title [MRG+1] Included new optional parameter in startproject command line Included new optional parameter in startproject command line Jul 11, 2016
Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
Signed-off-by: Felipe Ruhland <felipe.ruhland@gmail.com>
@feliperuhland
Copy link
Contributor Author

@feliperuhland feliperuhland commented Jul 19, 2016

@eliasdorneles I done what you suggested and I think it's good. Can you read the docstring to see if it's good enough?

Thanks again!

@eliasdorneles
Copy link
Member

@eliasdorneles eliasdorneles commented Jul 19, 2016

Yup, looks great now. Thank you, @feliperuhland !

@redapple this looks ready. How do you feel about merging it for 1.2?

@eliasdorneles eliasdorneles changed the title Included new optional parameter in startproject command line [MRG+1] Included new optional parameter in startproject command line Jul 19, 2016
@redapple redapple merged commit ec1c615 into scrapy:master Jul 19, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@redapple
Copy link
Contributor

@redapple redapple commented Jul 19, 2016

Thanks @feliperuhland !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.