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

feat: make ordered argument optional for workspace.getAllBlocks #7413

Closed
wants to merge 3 commits into from
Closed

feat: make ordered argument optional for workspace.getAllBlocks #7413

wants to merge 3 commits into from

Conversation

randrei12
Copy link
Contributor

If you're using typescript then you know how frustrating it is to always add a param when calling workspace.getAllBlocks(). That's why I think it's a good idea to make it optional. If a value is not passed, it will use false as default.

@randrei12 randrei12 requested a review from a team as a code owner August 19, 2023 17:08
@google-cla
Copy link

google-cla bot commented Aug 19, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome! It looks like this is your first pull request in Blockly, so here are a couple of tips:

  • You can find tips about contributing to Blockly and how to
    validate your changes on our
    developer site.

  • All contributors must sign the Google Contributor License
    Agreement (CLA). If the google-cla bot leaves a comment on this
    PR, make sure you follow the instructions.

  • We use conventional commits
    to make versioning the package easier. Make sure your commit
    message is in the proper format
    or learn how to fix it.

  • If any of the other checks on this PR fail, you can click on
    them to learn why. It might be that your change caused a test
    failure, or that you need to double-check the
    style guide.

Thank you for opening this PR! A member of the Blockly team will review it soon.

@rachel-fenichel
Copy link
Collaborator

This is a good idea, and just needs a few tweaks.

First, just adding the ? makes the parameter optional with a default value of undefined, per the TypeScript documentation. Please switch to getAllBlocks(ordered = false): Block[] { to actually give a default value of false.

Second, we'd actually like to do it for all four related methods in workspace.ts and their overrides in workspace_svg.ts:

  • getAllBlocks
  • getTopBlocks
  • getTopComments
  • getBlocksByType

Please let me know if you want to make this change for all of the methods. If not, I'll handle it as soon as I can merge your PR.

Finally, you'll need to change your PR title to feat: make ordered argument optional for workspace.getAllBlocks after you've added your next commit. You can just do that through the GitHub UI, or let me know if you're having issues with it.

Thanks!

@randrei12
Copy link
Contributor Author

I'll do the changes and also I can take care about the four functions.

@randrei12 randrei12 changed the title Make ordered argument optional at workspace.getAllBlocks feat: make ordered argument optional for workspace.getAllBlocks Aug 21, 2023
@conventional-commit-lint-gcf
Copy link

conventional-commit-lint-gcf bot commented Aug 21, 2023

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@github-actions github-actions bot added the PR: feature Adds a feature label Aug 21, 2023
@randrei12 randrei12 closed this Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: feature Adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants