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] fixed FEED_EXPORT_FIELDS handling (see #1223) #1224

Merged
merged 5 commits into from May 19, 2015

Conversation

@kmike
Copy link
Member

@kmike kmike commented May 12, 2015

A fix for #1223.

@@ -151,7 +151,7 @@ def __init__(self, settings):
if not self._exporter_supported(self.format):
raise NotConfigured
self.store_empty = settings.getbool('FEED_STORE_EMPTY')
self.export_fields = settings.getlist('FEED_EXPORT_FIELDS')
self.export_fields = settings.getlist('FEED_EXPORT_FIELDS') or None

This comment has been minimized.

@nramirezuy

nramirezuy May 12, 2015
Contributor

settings.getlist('FEED_EXPORT_FIELDS', None) cover the case of the user using an empty list.

This comment has been minimized.

@kmike

kmike May 12, 2015
Author Member

no, settings.getlist('FEED_EXPORT_FIELDS', None) returns []

This comment has been minimized.

@kmike

kmike May 12, 2015
Author Member

Currently all exporters but CSV exporter use None as a value indicating 'export fields are not specified', they handle [] as 'no fields'.

This comment has been minimized.

@curita

curita May 12, 2015
Member

So FEED_EXPORT_FIELDS can't be set to [] by the user with this change, right? I think it makes sense, not sure what's the actual use-case of that.

Maybe we should fix this in Settings, but I think the only way of knowing if the actual value was set as [] or isn't set at all is by checking with settings.get() before.

This comment has been minimized.

@kmike

kmike May 12, 2015
Author Member

So FEED_EXPORT_FIELDS can't be set to [] by the user with this change, right?

Yes; [] will be interpreted as None.

Maybe we should fix this in Settings, but I think the only way of knowing if the actual value was set as [] or isn't set at all is by checking with settings.get() before.

Yes, maybe it is better to fix it in settings, but fixing it in settings is more controversial and requires more discussion because it affects more code. getlist always returning a list kind of make sense, but on the other hand preserving default as-is also kind of make sense. We may use another sentinel instead of None for default default value to keep None - but does it worth the troubles, what are real use cases? That's what I mean by 'requires more discussion'.

This issue is a serious regression, so I think we should fix it without fixing issues with Settings if fixing Settings take time.

This comment has been minimized.

@curita

curita May 12, 2015
Member

Sorry, I meant that users won't be able to disable all fields to export, not that FEED_EXPORTS couldn't be set to [].

I agree with those Settings changes requiring more discussion, and using the current Settings implementation (with its limitations) for now.

Maybe we could check for explicit []s just to leave everyone happy?

if settings.get('FEED_EXPORT_FIELDS') is not None:
     self.export_fields = settings.getlist('FEED_EXPORT_FIELDS')
else:
     self.export_fields = None

This comment has been minimized.

@kmike

kmike May 12, 2015
Author Member

It was not possible to disable feed export fields by setting something to [] before introducing FEED_EXPORT_FIELDS option, and it is not possible now, so I don't see a problem :) But ok, I'll fix it.

This comment has been minimized.

@curita

curita May 12, 2015
Member

Oh, I was just following through @nramirezuy's comment, personally don't think it matters too much to have that option. If disabling feed export fields wasn't an option before we could merge the pr as it is I think.

This comment has been minimized.

@curita

curita May 12, 2015
Member

I left a +1 so you decide :P

This comment has been minimized.

@kmike

kmike May 12, 2015
Author Member

I've added [] support, for consistency.

@redapple
Copy link
Contributor

@redapple redapple commented May 12, 2015

LGTM

@curita
Copy link
Member

@curita curita commented May 12, 2015

+1

@kmike
Copy link
Member Author

@kmike kmike commented May 12, 2015

hmm, please not merge yet, CSV item exporter may handle [] differently

@kmike
Copy link
Member Author

@kmike kmike commented May 12, 2015

CSV exporter handles [] differently from other exporters since Scrapy 0.7 (see f3c6d83). Changing it would be technically a backwards-incompatible change. Without changing it the docs in this PR will be incorrect.

Opinions?

My vote is to change CSV exporter.

@kmike kmike added this to the Scrapy 1.0 milestone May 14, 2015
@kmike kmike force-pushed the fix-empty-feed-export-fields branch from b6a106d to 9b0ca1b May 18, 2015
@kmike
Copy link
Member Author

@kmike kmike commented May 18, 2015

After a discussion with @curita, @dangra, @pablohoffman and @eliasdorneles, I've made [] unsupported again.

@kmike kmike changed the title fixed FEED_EXPORT_FIELDS handling (see #1223) [MRG] fixed FEED_EXPORT_FIELDS handling (see #1223) May 18, 2015
dangra added a commit that referenced this pull request May 19, 2015
[MRG] fixed FEED_EXPORT_FIELDS handling (see #1223)
@dangra dangra merged commit ee59112 into master May 19, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@dangra dangra deleted the fix-empty-feed-export-fields branch May 19, 2015
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

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