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

Traversal functions use IteratorControl values rather than true/false #5857

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

ironage
Copy link
Contributor

@ironage ironage commented Sep 14, 2022

This change is for readability. Whenever I come across this pattern in the code I have to look up what true and false means contextually. Having a verbose type makes it easier to see what is going on without this mental indirection.

☑️ ToDos

  • 📝 Changelog update

@ironage ironage self-assigned this Sep 14, 2022
@cla-bot cla-bot bot added the cla: yes label Sep 14, 2022
Copy link
Contributor

@jedelbo jedelbo left a comment

Choose a reason for hiding this comment

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

Good idea. I also have trouble remembering what "true" means.

Copy link
Member

@nicola-cab nicola-cab left a comment

Choose a reason for hiding this comment

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

Nice, the only thing a bit odd is that we have got no testing around a simple thing like iterating through the tree. Although probably it is not really needed, since we never want to do that and many other parts of the code already exercise this logic.

@ironage ironage merged commit c4b790a into master Sep 15, 2022
@ironage ironage deleted the js/readability branch September 15, 2022 17:17
tgoyne added a commit that referenced this pull request Sep 16, 2022
…nification

* origin/master:
  Stop forcing enums to be 64 bits unnecessarily
  clean up documentation of internal fields in config structs
  SyncConfig should be default constructible
  Traversal functions use IteratorControl values rather than true/false which is more expressive (#5857)
  Fix handling of 4-byte UTF8 values on Windows (#5803)
  Encode links in a way the server can understand (#5835)
  expose `Group::remove_table` in the C API (#5860)
  Disable auto refresh for old realm during migration (#5856)
  Expose `list_find` in the c api (#5848)
  Do not allow asymmetric tables in pbs (#5853)
  Refactor link tracing code (#5796)
  Expose `Obj::get_parent_object` in the C API (#5851)
  Update app.hpp (#5854)
  Fix appending to list ignores existing query (#5850)
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants