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

Extract deceased field to its own function #136

Merged
merged 4 commits into from Jun 8, 2019

Conversation

Projects
None yet
2 participants
@anthonybaulo
Copy link
Contributor

commented Jun 6, 2019

Types of changes

  • Code cleanup / Refactoring

Description

Extract deceased field to its own function with tests.
Rename previous parse_deceased_field() to process_deceased_field()

Checklist:

  • I have updated the documentation accordingly
  • I have written unit tests

Fixes: #106

@rgreinho rgreinho self-requested a review Jun 7, 2019

@anthonybaulo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

@rgreinho
There's a lot going on with parsing the deceased field (L344-470 in apd.py), including a function named parse_deceased_field().
Is this issue solely concerned with extracting a parsing function for use inside of parse_page_content() on L504, which would literally just be moving the existing regex from the searches list into a new function?

searches = [
        (Fields.DECEASED, re.compile(r'>Deceased:\s*(?:</span>)?(?:</strong>)?\s*>?([^<]*\d)\s*.*\)?<')),
        (Fields.LOCATION, re.compile(r'>Location:.*>\s{2,}(?:</strong>)?([^<]+)')),
    ]

If so, what would you like to call the new function, since parse_deceased_field() is being used for something else?

@rgreinho

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

Oh you're right! so let's do the following:

  • Rename the existing parse_deceased_field() function to process_deceased_field(). Be sure to catch all the references to this functions. VS Code will be your friend :)
  • Name the new function you will create parse_deceased_field().

And we should be good to go!

Extract deceased parsing to its own function
The function parse_deceased_field() was created to pull content from the
corresponding field on the fatality page. There was an existing function
of the same name, which has now been renamed process_deceased_field().
Test cases have been made for the new parse_deceased_field().

Fixes #106

@anthonybaulo anthonybaulo marked this pull request as ready for review Jun 7, 2019

@rgreinho
Copy link
Member

left a comment

Very good job overall! Just one minor comment to check whether or not we succeeded to parse the field before adding it do the results.

Show resolved Hide resolved scrapd/core/apd.py Outdated
Add check for deceased field
The deceased field will not be created in the dictionary
unless a match was found.

Fixes #106
@rgreinho

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

And merged! Great work @anthonybaulo!

@mergify mergify bot merged commit 2fa140e into scrapd:master Jun 8, 2019

8 checks passed

Summary 1 rule matches
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: format Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: prepare Your tests passed on CircleCI!
Details
ci/circleci: test-integrations Your tests passed on CircleCI!
Details
ci/circleci: test-units Your tests passed on CircleCI!
Details
coverage/coveralls Coverage remained the same at 100.0%
Details

@anthonybaulo anthonybaulo deleted the anthonybaulo:issues/106/deceased-field branch Jun 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.