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 HDSpace pubdate parsing #5111

Merged
merged 13 commits into from
Sep 4, 2018
Merged

Fix HDSpace pubdate parsing #5111

merged 13 commits into from
Sep 4, 2018

Conversation

p0psicles
Copy link
Contributor

@p0psicles p0psicles commented Sep 4, 2018

  • PR is based on the DEVELOP branch
  • Don't send big changes all at once. Split up big PRs into multiple smaller PRs that are easier to manage and review
  • Read the contribution guide

fix #5100

@duramato

@p0psicles p0psicles added this to the 0.2.9 milestone Sep 4, 2018
p0psicles and others added 4 commits September 4, 2018 15:10
no need for log.exception here.
Cleanup the log.
calculate_date is not used anymore.
return datetime.now(tz.tzlocal()) - timedelta(seconds=seconds)

if fromtimestamp:
dt = datetime.fromtimestamp(int(pubdate), tz=tz.gettz('UTC'))
else:
dt = parser.parse(pubdate, dayfirst=df, yearfirst=yf, fuzzy=True)
from tzlocal import get_localzone
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove the import?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah

Copy link
Contributor

@sharkykh sharkykh Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p0psicles You marked as resolved but the import is still there
Edit: Marked as unresolved

@@ -574,12 +574,25 @@ def parse_pubdate(pubdate, human_time=False, timezone=None, **kwargs):
matched_time = int(round(float(matched_time.strip())))

seconds = parse('{0} {1}'.format(matched_time, matched_granularity))
if seconds is None:
log.warning('Failed parsing human time: {0} {1}', matched_time, matched_granularity)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the warning is needed since it's already going to cause an exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but up until now we often had to guess why

Copy link
Contributor

@sharkykh sharkykh Sep 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but now the exception includes the details you added to the warning log, so you don't really need the log?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exeception details are not logged down the stack.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see now. Sorry I missed that.

'expected': datetime.now().replace(microsecond=0, tzinfo=tz.gettz('UTC')) - timedelta(days=1, minutes=10, seconds=25),
'human_time': False
},
{ # p22: hd-space test human date like today at 12:00:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be # p23

@@ -127,6 +127,16 @@
'timezone': 'US/Eastern',
'fromtimestamp': True
},
{ # p22: hd-space test human date like yesterdat at 12:00:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: yesterday

@sharkykh sharkykh changed the title Feature/fix hd space Fix HDSpace pubdate parsing Sep 4, 2018
@sharkykh sharkykh added the Changelog Requires a changelog entry label Sep 4, 2018
@sharkykh sharkykh removed the Changelog Requires a changelog entry label Sep 4, 2018
Copy link
Contributor

@medariox medariox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@p0psicles
Copy link
Contributor Author

Just waiting for supers test/go

@sharkykh sharkykh added Needs testing Requires testing to make sure it's working as intended and removed Squash / Rebase labels Sep 4, 2018
@p0psicles p0psicles merged commit dfce238 into develop Sep 4, 2018
@p0psicles p0psicles deleted the feature/fix-hd-space branch September 4, 2018 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Concluded Needs testing Requires testing to make sure it's working as intended Provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants