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

Various small tickets... #7367

Merged
merged 10 commits into from
Jul 23, 2023
Merged

Various small tickets... #7367

merged 10 commits into from
Jul 23, 2023

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jul 21, 2023

Pull Request Description

  • Added Text.length into Text class so CB lists the built in.
  • Added File.starts_with and tests for the built in method.
  • Add to_js_object and to_display_text to Regex.
    image
  • Add to_js_object and to_display_text to Match.
    image
  • Remove the bit_shift_l alias from the built-ins.
  • Add test and Enso wrapper for Text.is_normalized.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@jdunkerley jdunkerley added the CI: No changelog needed Do not require a changelog entry for this PR. label Jul 21, 2023
@jdunkerley jdunkerley marked this pull request as ready for review July 21, 2023 14:57
_ ->
used_filter = if recursive.not || name_filter.contains "**" then name_filter else
Copy link
Contributor

Choose a reason for hiding this comment

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

contains or starts_with? If it has a ** in the middle but not at the start, will it still work right?

Copy link
Member Author

Choose a reason for hiding this comment

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

if it has ** in the middle I am trusting you know what you are doing!


"건반(Korean)".length
length : Integer
length self = @Builtin_Method "Text.length"
Copy link
Member

Choose a reason for hiding this comment

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

Advanturous! Static method is builtin, but instance method not and delegates to the static one. I am suprised it works. But if it does, then OK.

Copy link
Member

Choose a reason for hiding this comment

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

I think I don't understand, where is a static method here?

Comment on lines 729 to 730
filtered2 = root.list name_filter="*/*/*" recursive=True . map .to_text
filtered2.should_equal (resolve ["subdirectory/nested/b.txt"])
Copy link
Member

Choose a reason for hiding this comment

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

This case looks slightly worrying. If I understand correctly what happens, it will be prepended with **. So now it will find all paths that have >= 3 layers of nesting, whereas before this change it would only find paths that have == 3 layers of nesting.

Looking at the call, I'd indeed expect == 3 to be the correct behaviour here.

I appreciate the change, as indeed the other tests that you added show it was needed - specially the one just below - indeed if recursive=True, it's nice to find the file in all subdirectories. But it can break some usages for more advanced use cases. I'm not sure if we can do anything good about it though... It is not great that we cannot now use this method to find == n layers of nesting - we will always find arbitrarily nested files. On the other hand, maybe it's not that important - I think overall this change is a pragmatic improvement.

@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Jul 22, 2023
@mergify mergify bot merged commit 88f32d9 into develop Jul 23, 2023
24 of 26 checks passed
@mergify mergify bot deleted the wip/jd/minor-fixes branch July 23, 2023 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants