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

Update overview.rst | Fix an inconsistency #4213

Merged
merged 1 commit into from Dec 5, 2019
Merged

Conversation

wangqin0
Copy link
Contributor

@wangqin0 wangqin0 commented Dec 5, 2019

There exists an inconsistency between the code (line 37 - 38) and the output 'quotes.json' (line 56 - 68).

Note that even though according to line 53 - 54 'quotes.json' is "reformatted here for better readability", it cannot explain why the "author" field precedes the "text" field.

Intended output for the code BEFORE change:
[{
"text": "\u201cThe person, be it gentleman or lady, who has not pleasure in a good novel, must be intolerably stupid.\u201d",
"author": "Jane Austen"
},
{
"text": "\u201cOutside of a dog, a book is man's best friend. Inside of a dog it's too dark to read.\u201d",
"author": "Groucho Marx"
},
{
"text": "\u201cA day without sunshine is like, you know, night.\u201d",
"author": "Steve Martin"
},
...]

Intended output for the code After change (the inconsistency is fixed):
[{
"author": "Jane Austen",
"text": "\u201cThe person, be it gentleman or lady, who has not pleasure in a good novel, must be intolerably stupid.\u201d"
},
{
"author": "Groucho Marx",
"text": "\u201cOutside of a dog, a book is man's best friend. Inside of a dog it's too dark to read.\u201d"
},
{
"author": "Steve Martin",
"text": "\u201cA day without sunshine is like, you know, night.\u201d"
},
...]

There exists an inconsistency between the code (line 37 - 38) and the output 'quotes.json' (line 56 - 68). 

Note that even though according to line 53 - 54  'quotes.json' is "reformatted here for better readability", it cannot explain why the "author" field precedes the "text" field. 

Intended output for the code BEFORE change: 
    [{
        "text": "\u201cThe person, be it gentleman or lady, who has not pleasure in a good novel, must be intolerably stupid.\u201d",
        "author": "Jane Austen"
    },
    {
        "text": "\u201cOutside of a dog, a book is man's best friend. Inside of a dog it's too dark to read.\u201d",
        "author": "Groucho Marx"
    },
    {
        "text": "\u201cA day without sunshine is like, you know, night.\u201d",
        "author": "Steve Martin"
    },
    ...]

Intended output for the code After change (the inconsistency is fixed): 
    [{
        "author": "Jane Austen",
        "text": "\u201cThe person, be it gentleman or lady, who has not pleasure in a good novel, must be intolerably stupid.\u201d"
    },
    {
        "author": "Groucho Marx",
        "text": "\u201cOutside of a dog, a book is man's best friend. Inside of a dog it's too dark to read.\u201d"
    },
    {
        "author": "Steve Martin",
        "text": "\u201cA day without sunshine is like, you know, night.\u201d"
    },
    ...]
@codecov
Copy link

codecov bot commented Dec 5, 2019

Codecov Report

Merging #4213 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4213      +/-   ##
==========================================
- Coverage   83.74%   83.73%   -0.02%     
==========================================
  Files         165      165              
  Lines        9713     9713              
  Branches     1446     1446              
==========================================
- Hits         8134     8133       -1     
  Misses       1332     1332              
- Partials      247      248       +1
Impacted Files Coverage Δ
scrapy/utils/trackref.py 83.78% <0%> (-2.71%) ⬇️

@Gallaecio
Copy link
Member

Gallaecio commented Dec 5, 2019

CPython 3.5, which Scrapy will probably support until it reaches end-of-life (September) or a bit longer, does not keep insert order in dictionaries. So the output was probably copy-pasted from CPython 3.5 or earlier.

+1 to the change, though. Consistency is key!

@wRAR wRAR merged commit aaf94af into scrapy:master Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants