Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[video player]: update the url in the readme example #4081

Merged
merged 8 commits into from Jul 24, 2021
Merged

[video player]: update the url in the readme example #4081

merged 8 commits into from Jul 24, 2021

Conversation

maheshmnj
Copy link
Member

@maheshmnj maheshmnj commented Jun 23, 2021

This pull request updates the URL in the video_player sample code in the Readme.

Related issues:

  • This PR resolves the issue #84982

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Note that unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.
  • I updated CHANGELOG.md to add a description of the change.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

Thanks for the submission!

In the future, please don't remove the checklist from the template; it's there for a reason. This change needs to update the version so that it can be published, which was explained in that checklist.

(I think the better solution here would actually be to remove this section entirely, since pub.dev shows the example app's main file anyway, but this is fine as an incremental change.)

@maheshmnj
Copy link
Member Author

Thanks for the submission!

In the future, please don't remove the checklist from the template; it's there for a reason. This change needs to update the version so that it can be published, which was explained in that checklist.

(I think the better solution here would actually be to remove this section entirely, since pub.dev shows the example app's main file anyway, but this is fine as an incremental change.)

I am sorry I couldn't understand which checklist you are referring to could you please explain?

@stuartmorgan
Copy link
Contributor

When you created the PR, there was a template in the PR description section that should have been filled in; see almost any other PR in the repository for an example. You deleted all of that text rather than filling in the template and following the checklist that was part of it.

@maheshmnj
Copy link
Member Author

When you created the PR, there was a template in the PR description section that should have been filled in; see almost any other PR in the repository for an example. You deleted all of that text rather than filling in the template and following the checklist that was part of it.

Ohh got it, Sorry about that let me correct it,
I will make sure this doesn't happen on my future PRs :)
Let me know if there is anything else I need to fix.
Thanks

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Jun 25, 2021

Let me know if there is anything else I need to fix.

As I said above:

This change needs to update the version so that it can be published, which was explained in that checklist.

which is why I brought up the checklist in the first place.

@maheshmnj
Copy link
Member Author

Sorry about that again, so If I understand correctly these checks auto-publish the plugin to pub.dev which is why they are necessary.

@stuartmorgan
Copy link
Contributor

these checks

I'm not sure what "these checks" refers to.

which is why they are necessary.

Changing the version is necessary because we cannot publish updates to a package without giving the package a new version. Whether the publishing is automatic or manual isn't relevant here.

@maheshmnj
Copy link
Member Author

@stuartmorgan, Please let me know if there is anything I need to do from my side to get this merged.

Thank you for the review.

@stuartmorgan
Copy link
Contributor

@stuartmorgan, Please let me know if there is anything I need to do from my side to get this merged.

I've told you in every single comment above that you need to update the version; I'm not sure how I can be more clear about this.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

(Updating review state per comment above)


## 0.0.1

* Initial release
- Initial release
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you rewrite the entire changelog?

Copy link
Member Author

@maheshmnj maheshmnj Jul 23, 2021

Choose a reason for hiding this comment

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

I am really sorry about that actually its my IDE, when I saved the file, it auto-formatted the file. I have reverted the changes and pushed the updated changelog.
I have turned off the formatting now it won't happen in the future.

Copy link
Contributor

@stuartmorgan stuartmorgan left a comment

Choose a reason for hiding this comment

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

LGTM

@stuartmorgan stuartmorgan added waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. and removed last mile labels Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes p: video_player waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
3 participants