-
Notifications
You must be signed in to change notification settings - Fork 10.5k
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
Enable overriding TEMPLATE_DIR in startproject #1035
Conversation
|
||
@property | ||
def templates_dir(self): | ||
_templates_base_dir = self.settings['TEMPLATES_DIR'] or \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use self.setings.get('TEMPLATES_DIR', join(scrapy.__path__[0], 'templates'))
.
I feel like the default should be present on default_settings.py
I been doing some investigation on this.
This might be like a little confusing, so if you have doubts or comments don't hesitate to contact me. |
I have made changes to the code. Please have a look. Also, I am not sure where TEMPLATES_DIR_BASE will be used, since we aren't removing TEMPLATES_DIR from the default configuration (or are we?). This is why I didn't add the documentation for TEMPLATES_DIR_BASE. I will add it once we review the other things. |
3e39b24
to
af1a6c1
Compare
TEMPLATES_DIR = abspath(join(dirname(__file__), '..', 'templates')) | ||
|
||
TEMPLATES_DIR_BASE = abspath(join(dirname(__file__), '..', 'templates')) | ||
TEMPLATES_PROJECT_BASE='project' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Respect the spaces =
😄
I just added this to |
@@ -27,3 +27,18 @@ def string_camelcase(string): | |||
|
|||
""" | |||
return CAMELCASE_INVALID_CHARS.sub('', string.title()) | |||
|
|||
def get_base_template(settings,name="project"): | |||
base_dir = settings.get('TEMPLATES_DIR', settings.get('TEMPLATES_DIR_BASE')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function should look something like this:
from scrapy.utils.misc import args_to_iter
def get_template_dir(settings, name='project'):
if name == "project":
folder_name = settings.get('TEMPLATES_PROJECT', os.path.join(base_dir, settings['TEMPLATES_PROJECT_BASE']))
else:
folder_name = settings.get('TEMPLATES_SPIDERS', os.path.join(base_dir, settings['TEMPLATES_SPIDERS_BASE']))
if os.path.isabs(folder_name):
return folder_name
else:
for base_dir in args_to_iter(settings.get('TEMPLATES_DIR', settings['TEMPLATES_DIR_BASE'])):
path = os.path.join(base_dir, folder_name)
if os.path.exists(path):
return path
If you can make some tests for our new function, that would be great. |
af1a6c1
to
61e0857
Compare
__doctests__ = ['scrapy.utils.template'] | ||
|
||
|
||
class getTemplateDirTest(unittest.TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove get
How do you feel about supporting multiple folders? Our function returning existent folders and the command being able to look into these folders? /cc @SudShekhar @kmike |
Fix for Issue 671. Created a get_base_template and added tests for the same. Added 2 new settings to the docs and 3 to default_settings.py. Removed TEMPLATE_DIR setting.
61e0857
to
04390bd
Compare
|
||
TEMPLATES_DIR = abspath(join(dirname(__file__), '..', 'templates')) | ||
|
||
TEMPLATES_DIR_BASE = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that both of this path are the same. We need to remove one, I purpose the second one since it requires an extra import.
After one is removed, we have to add a path to the project in the position 0. I already found a way of getting it, but it needs more changes. So I will let you do some research, maybe you can find a better option. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into ways to do this. My idea would be to use os.getcwd() to get the directory where the script is running and add this to the user's version of scrapy.cfg (append at the end or something). Is this what you had in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But doesn't work if you are calling scrapy
command from inside the module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was suggesting that when we create the project, we add this setting to the scrapy.cfg (using startproject.py). So when we are running this command from inside the module (for genspider), you will automatically get the folder location as a part of template_dir_base
right? I guess I am a little confused by what you mean by module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nramirezuy : What if we keep only the first line in TEMPLATE_DIR_BASE and we modify the get_template_dir
function, to look through three stages:
TEMPLATES_PROJECT
-- template folder input given by user- For
startproject
, we look atabspath(join(dirname(__file__), '..', 'templates'))
[folder where we run the command]. Forgenspider
, we find closestscrapy.cfg
and use its directory. TEMPLATES_PROJECT_BASE
-> the original scrapy template folder.
Please let me know what do you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SudShekhar Since we are deprecating the _BASE
settings in favor of using priorities on Settings
, I think we should do the same. But still keep the setting as list.
Closing in favor of #1575 |
Fix for Issue #671.
Kindly let me know if there any changes are required.
Thanks!