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

Fix: String#tr implementation #770

Merged
merged 1 commit into from Aug 7, 2020
Merged

Conversation

dnamsons
Copy link
Contributor

@dnamsons dnamsons commented Aug 7, 2020

No description provided.

@@ -687,12 +687,11 @@ def to_str
end

def tr(from_str, to_str)
# TODO: Support character ranges c1-c2
Copy link
Contributor Author

@dnamsons dnamsons Aug 7, 2020

Choose a reason for hiding this comment

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

It seems like support for character ranges has been implemented already, therefore i removed this TODO comment

Copy link
Member

Choose a reason for hiding this comment

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

The below use of Regexp is a hack. This does method does not properly validate that ranges are ascending.

For example, this must error:

[2.6.6] > "hello".tr("a-y", "z-b")
Traceback (most recent call last):
        5: from /usr/local/var/rbenv/versions/2.6.6/bin/irb:23:in `<main>'
        4: from /usr/local/var/rbenv/versions/2.6.6/bin/irb:23:in `load'
        3: from /usr/local/var/rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        2: from (irb):6
        1: from (irb):6:in `tr'
ArgumentError (invalid range "z-b" in string transliteration)

Can you restore the comment?

@@ -19,6 +19,7 @@
it "pads to_str with its last char if it is shorter than from_string" do
"this".tr("this", "x").should == "xxxx"
"hello".tr("a-z", "A-H.").should == "HE..."
"abcd".tr("a-z", "xxx").should == "xxxx"
Copy link
Member

Choose a reason for hiding this comment

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

ruby-specs come from upstream. I'd consider this source file immutable. Can you back out this change?

If you think we're missing a test, add an integration test for String like these for Array:

#[cfg(test)]
mod tests {
use bstr::ByteSlice;
use crate::test::prelude::*;
const FUNCTIONAL_TEST: &[u8] = include_bytes!("array_functional_test.rb");
#[test]
fn functional_test() {
let mut interp = crate::interpreter().unwrap();
let _ = interp.eval(FUNCTIONAL_TEST).unwrap();
let result = interp.eval(b"spec");
if let Err(exc) = result {
let backtrace = exc.vm_backtrace(&mut interp);
let backtrace = bstr::join("\n", backtrace.unwrap_or_default());
panic!(
"Array tests failed with backtrace:\n{:?}",
backtrace.as_bstr()
);
}
}
}

https://github.com/artichoke/artichoke/blob/b99e3c1163381cec8ab505e33c2201dbe67c7ade/artichoke-backend/src/extn/core/array/array_functional_test.rb

@@ -687,12 +687,11 @@ def to_str
end

def tr(from_str, to_str)
# TODO: Support character ranges c1-c2
Copy link
Member

Choose a reason for hiding this comment

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

The below use of Regexp is a hack. This does method does not properly validate that ranges are ascending.

For example, this must error:

[2.6.6] > "hello".tr("a-y", "z-b")
Traceback (most recent call last):
        5: from /usr/local/var/rbenv/versions/2.6.6/bin/irb:23:in `<main>'
        4: from /usr/local/var/rbenv/versions/2.6.6/bin/irb:23:in `load'
        3: from /usr/local/var/rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        2: from (irb):6
        1: from (irb):6:in `tr'
ArgumentError (invalid range "z-b" in string transliteration)

Can you restore the comment?

@lopopolo
Copy link
Member

lopopolo commented Aug 7, 2020

I tried this commit locally, and while it does improve spec compliance, there are still many ruby-spec failures. I'd like to take this PR with the changes I've requested.

Because String#tr still has some work to do, can you also edit the issue description to remove the "autoclose" annotation so GitHub does not close the attached ticket? Linking to it is fine.

@lopopolo
Copy link
Member

lopopolo commented Aug 7, 2020

Please squash and force push your changes so we don't add any extraneous commits to the specs directory.

@dnamsons
Copy link
Contributor Author

dnamsons commented Aug 7, 2020

@lopopolo Thank you for the review!

I've applied the requested changes

@@ -42,4 +43,8 @@ def string_unary_minus
raise unless s.itself == 'abababa'
end

def string_tr
Copy link
Member

Choose a reason for hiding this comment

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

oh I didn't realize we had one of these for String 🚀

@lopopolo lopopolo merged commit 039f1a0 into artichoke:master Aug 7, 2020
@lopopolo
Copy link
Member

lopopolo commented Aug 7, 2020

link to #765

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ruby-core Area: Ruby Core types.
Development

Successfully merging this pull request may close these issues.

None yet

2 participants