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

docs: Update flame_audio readme #3119

Merged
merged 7 commits into from
Apr 9, 2024

Conversation

xjyribro
Copy link
Contributor

@xjyribro xjyribro commented Apr 9, 2024

Description

Closes: #2511

Checklist

  • I have followed the Contributor Guide when preparing my PR.
  • I have updated/added tests for ALL new/updated/fixed functionality.
  • I have updated/added relevant documentation in docs and added dartdoc comments with ///.
  • I have updated/added relevant examples in examples or docs.

Breaking Change?

  • Yes, this PR is a breaking change.
  • No, this PR is not a breaking change.

Related Issues

@xjyribro
Copy link
Contributor Author

xjyribro commented Apr 9, 2024

@spydon

For issue #2511

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Nice! It would be good to have an example of how to use AudioPool too, and there are some issues that the markdown linter complains on that should be fixed:

 packages/flame_audio/README.md:32 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 2; Actual: 1; Above] [Context: "## How to use"]
packages/flame_audio/README.md:41 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 2; Actual: 1; Above] [Context: "### General sounds"]
packages/flame_audio/README.md:61 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 2; Actual: 1; Above] [Context: "### Background music"]
packages/flame_audio/README.md:63:38 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
packages/flame_audio/README.md:64 MD031/blanks-around-fences Fenced code blocks should be surrounded by blank lines [Context: "```dart"]
packages/flame_audio/README.md:96 MD022/blanks-around-headings Headings should be surrounded by blank lines [Expected: 2; Actual: 1; Above] [Context: "### Caching"]
packages/flame_audio/README.md:98:101 MD013/line-length Line length [Expected: 100; Actual: 172]
packages/flame_audio/README.md:119 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 2; Actual: 3]

packages/flame_audio/README.md Outdated Show resolved Hide resolved
packages/flame_audio/README.md Outdated Show resolved Hide resolved
../../README.md
Copy link
Member

@spydon spydon Apr 9, 2024

Choose a reason for hiding this comment

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

What changed here?

Copy link
Member

Choose a reason for hiding this comment

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

Probably just revert this change, I'm guessing it is some change of the symlink?
The MD linter crashes on it now:

Error: ENOENT: no such file or directory, open 'packages/flame/README.md'
    at Object.openSync (node:fs:596:3)
    at Object.readFileSync (node:fs:464:35)
    at lintFile (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/node_modules/markdownlint/lib/markdownlint.js:852:33)
    at lintWorker (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/node_modules/markdownlint/lib/markdownlint.js:937:7)
    at lintInput (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/node_modules/markdownlint/lib/markdownlint.js:978:7)
    at Function.markdownlintSync [as sync] (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/node_modules/markdownlint/lib/markdownlint.js:1027:3)
    at lintAndPrint (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/markdownlint.js:319:35)
    at Object.<anonymous> (/opt/hostedtoolcache/node/18.20.1/x64/lib/node_modules/markdownlint-cli/markdownlint.js:325:5)
    at Module._compile (node:internal/modules/cjs/loader:1364:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1422:10) {
  errno: -2,
  syscall: 'open',
  code: 'ENOENT',
  path: 'packages/flame/README.md'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't maually edit this file. I ran melos markdown-fix. I believe it fixed this error: packages\flame\README.md:1:15 MD047/single-trailing-newline Files should end with a single newline character

Copy link
Member

Choose a reason for hiding this comment

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

Alright, interesting that it doesn't complain about that in the pipeline, probably some platform differences. If you just checkout the main version it will pass in the pipeline.

Copy link
Member

Choose a reason for hiding this comment

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

Alright, I fixed the readme file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm on a Windows PC so maybe some differences...

@spydon
Copy link
Member

spydon commented Apr 9, 2024

@xjyribro do you want to add an AudioPool example, or should be merge this as-is?

@xjyribro
Copy link
Contributor Author

xjyribro commented Apr 9, 2024

I've not used AudioPools before so I created the example based on my understanding of the docs. 06bee31

Copy link
Member

@spydon spydon left a comment

Choose a reason for hiding this comment

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

Lgtm

@spydon spydon merged commit 843984d into flame-engine:main Apr 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update flame_audio readme
2 participants