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

Removed an invalid test case from isCurrentPostScheduled. #5858

Merged
merged 1 commit into from
Mar 28, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Mar 28, 2018

The test case "should return true for posts with status future" was invalid as besides the status being future the scheduled date needs also to be in the future otherwise the post is not "scheduled" but "published".
During the development of the selector, I missed to removal of test case as it should have been replaced by "should return true for future scheduled posts".

How Has This Been Tested?

Execute the automated sometimes and verify the tests pass.

The test case "should return true for posts with status future" was invalid as besides the status being future the schedule date needs also to be in the future otherwise the post is not scheduled but published.
@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Mar 28, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this Mar 28, 2018
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

👍

@jorgefilipecosta jorgefilipecosta merged commit 2ae2108 into master Mar 28, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/isCurrentPostScheduled-test-case branch March 28, 2018 19:59
@gziolo gziolo mentioned this pull request Mar 29, 2018
@danielbachhuber
Copy link
Member

@aduth Can we make sure merged pull requests are assigned to a milestone? Without milestone assignment, it's more difficult to know whether a pull request or issue has made it into a release.

@jorgefilipecosta jorgefilipecosta added this to the 2.6 milestone Mar 29, 2018
@aduth
Copy link
Member

aduth commented Mar 29, 2018

@danielbachhuber Hmm, it adds decent overhead to the merge process; admittedly becoming routine for an individual, but difficult to coordinate across an organization.

Is this not something that can be determined by the commits occurrence relative to the release tags? Or, at worst, automated by a bot.

@danielbachhuber
Copy link
Member

Hmm, it adds decent overhead to the merge process; admittedly becoming routine for an individual, but difficult to coordinate across an organization.

Communication is the oxygen of a distributed (company|team|project).

Is this not something that can be determined by the commits occurrence relative to the release tags?

Not as easily, to my knowledge. Can you tell me how to find the release tag for #5549 ?

Or, at worst, automated by a bot.

If there is such a bot, we could use a bot. I haven't come across one, though.

@aduth
Copy link
Member

aduth commented Mar 29, 2018

Communication is the oxygen of a distributed (company|team|project).

Eloquent. But in this case, this is just a waste of time.

Not as easily, to my knowledge. Can you tell me how to find the release tag for #5549 ?

git describe --contains looks like a good fit.

Find the merge commit:

image

Then:

git describe --contains 8a4d644
# v2.4.0~30

@danielbachhuber
Copy link
Member

But in this case, this is just a waste of time.

We can agree to disagree then.

If you can take 30 seconds to write a helpful commit message for others working on the project, you can take an extra 2 seconds to assign a pull request to a milestone for the same purpose. Not assigning a pull request to a milestone adds easily 30 seconds overhead for anyone technically capable enough to figure it out after the fact.

In WordPress core development, Trac issues are assigned to milestones for a very good reason: to preserve the historical legacy of what code landed when. As Gutenberg opens up to a multitude of new users via Try Gutenberg, it would be very helpful for all project contributors to more easily reference what has landed when.

As it stands, your suggestion means a potential Gutenberg contributor has to know Git at the command line in order to track whether or not a fix has been released, etc. Subsequently, this excludes a larger majority of the WordPress community who can help us triage the influx of new issues.

@aduth
Copy link
Member

aduth commented Mar 29, 2018

But it's not two seconds. It's the cumulative hours we spend discussing whether this is worth the effort (as we are doing at this very moment), then documenting, communicating, and enforcing the workflow. I'm not the only person doing code merges.

I'm not saying there's not value in it. "Waste" was perhaps overly reactionary to the quoted rhetoric, but there is an opportunity cost here to consider, and its overall value to moving the project forward ought to be weighed with this in mind.

@danielbachhuber
Copy link
Member

It's the cumulative hours we spend discussing whether this is worth the effort (as we are doing at this very moment), then documenting, communicating, and enforcing the workflow. I'm not the only person doing code merges.

This seems like a solvable problem #5887

@mtias
Copy link
Member

mtias commented Apr 2, 2018

As the person writing most release changelogs, this could be helpful to my flow, but at the same time, without certainty that this always happens on merge, I'll still be crawling through all changes since previous release to compile the list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants