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.

Sign up for GitHub

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

Jump to bottom

Add SWITCH #16

Merged
merged 11 commits into from
Feb 9, 2024
Merged

Add SWITCH #16

merged 11 commits into from
Feb 9, 2024

Conversation

kashimoto-noriko
Copy link

@kashimoto-noriko kashimoto-noriko commented Feb 6, 2024 •
edited
Loading

Background

It is difficult to produce the required output with the current function.
This can be solved by using SWITCH function.

Changes

  • add SWITCH function
XlsFunction.evaluate('SWITCH("abc", "abc", "yes","not")')                                                      
=> "yes"  
XlsFunction.evaluate('SWITCH("abc", "ac", "yes","not")')                                                   
=> "not"
XlsFunction.evaluate('SWITCH("abc", "ac", "yes")')
=> "#N/A"

If there are more than 126 arguments and there is no matching value up to the 126th argument, returns "#N/A".

kashimoto-noriko
kashimoto-noriko commented Feb 6, 2024
View reviewed changes
lib/xls_function/evaluators/functions/switch.rb Outdated
class Switch < ::XlsFunction::Evaluators::FunctionEvaluator
function_as :switch

MAX_ARGUMENTS_COUNT = 254
Copy link
Author

Choose a reason for hiding this comment

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

MAX_ARGUMENTS_COUNT is total value below.

  • Value to switch (1)
  • What value do you want to match (126 * 2 = 252)
  • Default value to return if there's no match found (1)

@kashimoto-noriko kashimoto-noriko marked this pull request as ready for review February 6, 2024 06:14
yiyenene
yiyenene reviewed Feb 6, 2024
View reviewed changes
Copy link
Member

@yiyenene yiyenene left a comment

Choose a reason for hiding this comment

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

@kashimoto-noriko 🙏

lib/xls_function/evaluators/functions/switch.rb Outdated
Comment on lines 8 to 11
MAX_ARGUMENTS_COUNT.times do |n|
text_name = :"condition#{n + 1}"
define_arg text_name
end
Copy link
Member

Choose a reason for hiding this comment

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

You should take efficiency into account because this function has variable arguments.
Could you try to re-consider implementing as short-circuit ? Please refer to the ifs function.

Copy link
Author

Choose a reason for hiding this comment

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

I fixed it using ifs as a reference.
9a6db25

@MurakiSari
Copy link
Member

半分くらい理解できて半分くらいわからないので明日聞く😇

kimdj2
kimdj2 reviewed Feb 7, 2024
View reviewed changes
Copy link
Member

@kimdj2 kimdj2 left a comment

Choose a reason for hiding this comment

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

1点だけコメントしました🙏

lib/xls_function/evaluators/functions/switch.rb Outdated

def not_match(args)
args.delete_at(1)
args.length.odd? ? args.last : nil
Copy link
Member

Choose a reason for hiding this comment

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

[質問]
こちらargs.delete_at(1)する理由とかありますかね？
delete_atしなくて

args.length.even? ? args.last : nil

↑でいい気がしまして、、

Copy link
Author

Choose a reason for hiding this comment

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

I fixed it using ifs as a reference.
9a6db25

Copy link
Member

Choose a reason for hiding this comment

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

It's becoming more easier to understand!

kimdj2
kimdj2 approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@kimdj2 kimdj2 left a comment

Choose a reason for hiding this comment

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

LGTM!

yiyenene
yiyenene reviewed Feb 8, 2024
View reviewed changes
Copy link
Member

@yiyenene yiyenene left a comment

Choose a reason for hiding this comment

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

@kashimoto-noriko 🙏

lib/xls_function/evaluators/functions/switch.rb Outdated
Comment on lines 15 to 17
return value&.evaluate(context) if expr&.evaluate(context) == condition

return expr&.evaluate(context) if value&.evaluate(context).nil?
Copy link
Member

Choose a reason for hiding this comment

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

expr&.evaluate(context) runs twice. Plz fix it.

Copy link
Author

Choose a reason for hiding this comment

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

I fixed in the commit below.
b066e5d

lib/xls_function/evaluators/functions/switch.rb Outdated

arg_list[1..-1].each_slice(2) do |expr, value|
count += 1
break if count > 126
Copy link
Member

Choose a reason for hiding this comment

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

[imo] Original one is restricted to 126 as you implemented. But I think our function does not need to be restricted as well. I don't restrict similar functions such as ifs and let. How do you think ?

Copy link
Author

Choose a reason for hiding this comment

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

First of all, I tried to reproduce it faithfully, but I also think that restrictions are unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

I deleted it in the commit below.
4a009cd

lib/xls_function/evaluators/functions/switch.rb Outdated
return expr&.evaluate(context) if value&.evaluate(context).nil?
end

XlsFunction::ErrorValue.na
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to add some messages with class_info. See also other ErrorValue usage.

Copy link
Author

Choose a reason for hiding this comment

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

I added a message in the commit below.
a230f0f

MurakiSari
MurakiSari reviewed Feb 8, 2024
View reviewed changes
lib/xls_function/evaluators/functions/switch.rb
end

XlsFunction::ErrorValue.na
end
Copy link
Member

Choose a reason for hiding this comment

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

🗒

  • 基本的に関数追加する場合は、 eval メソッドの中に関数処理を実装すれば良い
  • arg_list で入力した式をいい感じに取得できる

    xls_function/lib/xls_function/evaluators/function_evaluator.rb

    Lines 16 to 18 in 3b304f6

    def arg_list
    @arg_list ||= Array(context[:arglist])
    end
    • expr: 一致する値、 value: 一致した時に出力したい値、が入る想定
    • value&.evaluate(context).nil?　であれば、最後の一致する値がない場合に返す値とみなしている

yiyenene
yiyenene approved these changes Feb 8, 2024
View reviewed changes
Copy link
Member

@yiyenene yiyenene left a comment

Choose a reason for hiding this comment

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

LGTM

@yiyenene yiyenene merged commit b1adac9 into main Feb 9, 2024
4 checks passed
@yiyenene yiyenene deleted the issue_52995 branch February 9, 2024 04:17
@yiyenene yiyenene mentioned this pull request Feb 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MurakiSari MurakiSari MurakiSari left review comments

@yiyenene yiyenene yiyenene approved these changes

@kimdj2 kimdj2 kimdj2 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kashimoto-noriko @MurakiSari @yiyenene @kimdj2