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] Fix JsonWriterPipeline example in docs #2302

Merged
merged 2 commits into from Oct 18, 2016

Conversation

@Granitosaurus
Copy link
Contributor

@Granitosaurus Granitosaurus commented Oct 3, 2016

The current example does not save the file properly if crawler is called from within a script since file object is never closed.

related stackoverflow question: http://stackoverflow.com/questions/39821693/scrapy-print-pipeline-data-in-a-scripts-context

@codecov-io
Copy link

@codecov-io codecov-io commented Oct 3, 2016

Current coverage is 83.31% (diff: 100%)

Merging #2302 into master will increase coverage by 3.34%

@@             master      #2302   diff @@
==========================================
  Files           161        161          
  Lines          8719       8719          
  Methods           0          0          
  Messages          0          0          
  Branches       1284       1284          
==========================================
+ Hits           6972       7264   +292   
+ Misses         1496       1204   -292   
  Partials        251        251          

Powered by Codecov. Last update 3235bfe...dfba151

@kmike
Copy link
Member

@kmike kmike commented Oct 5, 2016

Hey @Granitas,

Yeah, the example is not correct; this is mentioned in the note to the next example. It is better to either to fix the example and remove the note, or move the note closer to JsonWriterPipeline example.

@redapple
Copy link
Contributor

@redapple redapple commented Oct 5, 2016

@Granitas , the example definitely needs fixing, thanks for this.
I would keep the note. I see no harm in it.

@kmike
Copy link
Member

@kmike kmike commented Oct 5, 2016

@redapple I'm talking about this note:

Previous example (JsonWriterPipeline) doesn't clean up resources properly.
Fixing it is left as an exercise for the reader.

@redapple
Copy link
Contributor

@redapple redapple commented Oct 5, 2016

Oh, right, I didn't even see the note on GitHub.
Good to remove.

@Granitosaurus
Copy link
Contributor Author

@Granitosaurus Granitosaurus commented Oct 5, 2016

Got rid of the comment, it was really weirdly placed so I didn't even notice it.

@redapple redapple changed the title fix JsonWriterPipeline example in docs [MRG+1] Fix JsonWriterPipeline example in docs Oct 12, 2016
@kmike kmike merged commit a5f4450 into scrapy:master Oct 18, 2016
3 checks passed
3 checks passed
codecov/patch Coverage not affected when comparing 3235bfe...dfba151
Details
codecov/project 83.31% (+3.34%) compared to 3235bfe
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kmike
Copy link
Member

@kmike kmike commented Oct 18, 2016

Thanks!

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

4 participants