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

Fix two bugs in scraper code. #3063

Merged
merged 3 commits into from May 15, 2019

Conversation

Projects
None yet
5 participants
@Brhstone
Copy link

commented Apr 19, 2019

Fixed parser failing due to missing directory (#2684) and when milliseconds were less than 100 (#1954).

Description

Addressed two issues:

  • #2684 Fixed by catching the HTTP error and ignoring it if the error code was 404 (Not Found). Added a test asserting the fix.
  • #1954 Fixed the randomly failing test by addressing the underlying bug -- milliseconds were not being zero-padded. The _URL_followsPattern logic relies on URL length, which changes if zero-padding isn't used. The randomness was due to the random chance of the current milliseconds time being less than 100 (derived from microseconds). Added a test asserting the fix by using a mocked datetime of the edge case. I was not able to get the two other tests mentioned in the ticket to fail.

Fixes #2684 and fixes #1954

Brandon Stone
@sunpy-bot

This comment has been minimized.

Copy link

commented Apr 19, 2019

Thanks for the pull request @Brhstone! Everything looks great!

@nabobalis nabobalis added this to the 0.9.7 milestone Apr 19, 2019

@nabobalis nabobalis requested a review from Cadair Apr 19, 2019

@pytest.mark.xfail
def testFilesRange_sameDirectory_local():
# Fails due to an IsADirectoryError, wrapped in a URLError, after `requests`
# tries to open a directory as a binary file.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Apr 19, 2019

Contributor

So this is something we can fix or should this test actually always fail?

This comment has been minimized.

Copy link
@Brhstone

Brhstone Apr 19, 2019

Author

Excellent question. I don't know if scanning directories was a functioning feature at some point or not. It's definitely fixable/implementable, but it'd take some effort.

This comment has been minimized.

Copy link
@nabobalis

nabobalis Apr 19, 2019

Contributor

Yeah, that does seem like an undertaking. I wonder if we want to support that in the future or just drop it. What do you think @Cadair?

This comment has been minimized.

Copy link
@Cadair

Cadair Apr 23, 2019

Member

I honestly have no idea. If there isn't an issue for this already we should probably open one. Also I am guessing @dpshelio wrote this test.

This comment has been minimized.

Copy link
@samaloney

samaloney Apr 25, 2019

Contributor

There is already a check for ftp type urls (uris? I never know which) maybe add a check for file type urls and raise not implemented exception and alter the test to match? I mean it can't work as as it is?

@Cadair Cadair changed the title Fixed parser failing due to missing directory (#2684) and … Fix two bugs in scraper code. Apr 23, 2019

@Cadair

This comment has been minimized.

Copy link
Member

commented Apr 23, 2019

Thanks for the patch @Brhstone 😄

I think this is good to go, if you can add a changelog entry (one per bug fix) as per the instructions in the changelog readme that would be awesome.

@Cadair Cadair modified the milestones: 0.9.7, 0.9.8 May 1, 2019

Cadair added some commits May 15, 2019

@Cadair Cadair modified the milestones: 0.9.9, 0.9.8 May 15, 2019

@Cadair

Cadair approved these changes May 15, 2019

@Cadair Cadair merged commit 984e99c into sunpy:master May 15, 2019

15 of 16 checks passed

codecov/patch 83.33% of diff hit (target 88.23%)
Details
ci/circleci: 32bit Your tests passed on CircleCI!
Details
ci/circleci: egg-info-36 Your tests passed on CircleCI!
Details
ci/circleci: egg-info-37 Your tests passed on CircleCI!
Details
ci/circleci: figure-tests-36 Your tests passed on CircleCI!
Details
ci/circleci: html-docs Your tests passed on CircleCI!
Details
ci/circleci: pip-install Your tests passed on CircleCI!
Details
codecov/project 89.91% (+1.68%) compared to c26fa0e
Details
giles Click details to preview the documentation build
Details
sunpy-bot All checks passed
sunpy.sunpy Build #20190515.3 succeeded
Details
sunpy.sunpy (Linux_36_Conda_offline) Linux_36_Conda_offline succeeded
Details
sunpy.sunpy (Linux_36_offline) Linux_36_offline succeeded
Details
sunpy.sunpy (Linux_37_online) Linux_37_online succeeded
Details
sunpy.sunpy (Windows_36_offline) Windows_36_offline succeeded
Details
sunpy.sunpy (macOS_37_offline) macOS_37_offline succeeded
Details

nabobalis added a commit to nabobalis/sunpy that referenced this pull request May 30, 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.