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

adding condition to run LastModified test #2308

Merged
merged 1 commit into from Nov 11, 2015

Conversation

martiansideofthemoon
Copy link
Contributor

I hope the changes are okay @jgraham . Could someone tell me the reason why I have the extra diff under tools? Is it because I didn't run git submodule update --init --recursive again after pulling from upstream ? Trying to solve #2300

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/5940

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.


// Checking whether d and and new instance of Date have the same timezone.
// Do not run the time zone test in the case daylight saving occurs.
var d2 = new Date();
Copy link
Contributor

Choose a reason for hiding this comment

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

probably should move these two statements after the next two ones. shouldn't affect anything, but is clearer when date_now is the last thing executed, and future-proof

@martiansideofthemoon
Copy link
Contributor Author

@Manishearth adding the changes

// Do not run the time zone test in the case daylight saving occurs.
var last_modified_date = new Date(last_modified);
var d2 = new Date();
if (d2.getTimezoneOffset()==last_modified_date.getTimezoneOffset()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spaces around operators

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Manishearth removed the nit. Why has the tools file been edited in my commit?

@Manishearth
Copy link
Contributor

You messed up the submodule. cd to the submodule folder, git reset --hard d93ad88336e5933b158129596c196d568ae15f82, git add the submodule folder and git commit --amend. In the future, ensure there aren't any submodule changes before using git commit -a or git add *

(Also, squash the rest of your commits)

@martiansideofthemoon
Copy link
Contributor Author

@Manishearth I hope it is okay.

// Do not run the time zone test in the case daylight saving occurs.
var last_modified_date = new Date(last_modified);
var d2 = new Date();
if (d2.getTimezoneOffset() == last_modified_date.getTimezoneOffset()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No, just check if d2 and d have different timezones

@Manishearth
Copy link
Contributor

r+

@Manishearth
Copy link
Contributor

merge? @jgraham

jgraham added a commit that referenced this pull request Nov 11, 2015
adding condition to run LastModified test
@jgraham jgraham merged commit 020a5eb into web-platform-tests:master Nov 11, 2015
@martiansideofthemoon martiansideofthemoon deleted the my-code-fix branch November 11, 2015 17:25
@tobie tobie added the html label Nov 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants