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(stdlib): extract function #587

Merged
merged 19 commits into from
Nov 30, 2024
Merged

Conversation

Mte90
Copy link
Member

@Mte90 Mte90 commented Nov 12, 2024

Ref: #449

So I implemented the extract function in amber, required a new function contains_multiple

@Mte90 Mte90 requested review from hdwalters and Ph0enixKM November 12, 2024 11:18
@Mte90 Mte90 changed the title feat(stdlib): yeah feat(stdlib): extract function Nov 12, 2024
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/tests/stdlib/contains_multiple.ab Outdated Show resolved Hide resolved
src/tests/stdlib/contains_multiple.ab Outdated Show resolved Hide resolved
src/tests/stdlib/extract.ab Outdated Show resolved Hide resolved
src/tests/stdlib/extract.ab Outdated Show resolved Hide resolved
@Mte90 Mte90 requested a review from hdwalters November 14, 2024 10:03
@Mte90
Copy link
Member Author

Mte90 commented Nov 14, 2024

Implemented new functions for contains and also the regex one with their tests.

src/std/text.ab Show resolved Hide resolved
src/tests/stdlib/extract.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
Co-authored-by: Phoenix Himself <[email protected]>
@Mte90 Mte90 requested a review from Ph0enixKM November 14, 2024 14:26
src/std/fs.ab Outdated Show resolved Hide resolved
Co-authored-by: Phoenix Himself <[email protected]>
@Mte90 Mte90 requested a review from Ph0enixKM November 14, 2024 16:40
src/std/text.ab Show resolved Hide resolved
src/std/text.ab Outdated
let flag = status == 0 then "-r" else "-E"
output = $ echo "{source}" | sed "{flag}" -e "{search}" $
} else {
output = $ echo "{source}" | sed -e "{search}" $
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to use sed -n (suppress automatic printing), and /.../p to print matching lines:

...
    output = $ echo "{source}" | sed "{flag}" -ne "/{search}/p" $
} else {
    output = $ echo "{source}" | sed -ne "/{search}/p" $
...

Copy link
Member Author

Choose a reason for hiding this comment

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

it is working as it is now, if I change in that way doesn't work

src/std/text.ab Outdated
} else {
output = $ echo "{source}" | sed -e "{search}" $
}
if output == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to return true if sed output is not empty:

if output != "" {
    return true
}

Copy link
Contributor

@hdwalters hdwalters Nov 19, 2024

Choose a reason for hiding this comment

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

See above for reasons why it's still recommended to use if output != "".

src/std/fs.ab Outdated Show resolved Hide resolved
src/tests/stdlib/contains_any.ab Show resolved Hide resolved
src/tests/stdlib/contains_all.ab Show resolved Hide resolved
src/tests/stdlib/contains_any_regex.ab Outdated Show resolved Hide resolved
src/tests/stdlib/contains_regex.ab Outdated Show resolved Hide resolved
Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

I agree with @hdwalters suggestions

@Mte90
Copy link
Member Author

Mte90 commented Nov 15, 2024

I have some doubts about removing /../ as I think that should be the user to choose how to write the regular expression.

@Mte90 Mte90 requested a review from Ph0enixKM November 15, 2024 10:31
src/std/text.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
Mte90 and others added 3 commits November 15, 2024 17:03
@hdwalters
Copy link
Contributor

hdwalters commented Nov 15, 2024

@Mte90 wrote:

I have some doubts about removing /../ as I think that should be the user to choose how to write the regular expression.

I still can't agree with this; I maintain that calling functions should pass regex not /regex/g.

  1. The slashes are delimiters for the regular expression; they are not part of the expression itself. In all the languages I'm familiar with, regular expressions are passed as quote delimited strings; requiring users to pass slashes as well is just going to cause confusion. And the "global" modifier only makes sense in a replace, not a search context.

  2. While we currently implement regular expression support using sed, this may change in the future. If we force scripts to pass slashes as well, and the hypothetical new method does not use exactly the same syntax as sed, those scripts will break.

  3. If we want to allow alternate regular expression functionality (case insensitive match comes to mind; sed does this with /regex/I) we should add new standard library function parameters, and build the sed command inside the function. This would allow us to replace sed without breaking existing scripts.

@hdwalters
Copy link
Contributor

@Ph0enixKM wrote:

is there any way to tell sed that we will pass a regex string without these backslashes? Or to preprocess the string by replacing all the symbols like (, |, + etc. with ones added with prefix \\?

We have to escape those operators with basic regex support in sed; extended regex support can be enabled by passing true to the standard library function.

I don't think we should do any preprocessing on the string; we can't predict what various flavours of sed will do; and preprocessing will just confuse the issue. Any differences in sed support on different operating systems would be problems developers would have to resolve in native Bash scripts; I do not think it's up to us to fix that.

@hdwalters
Copy link
Contributor

However, it occurs to me that we can support any hypothetical move away from sed for regular expressions in future, by renaming the standard library functions as match_sed and replace_sed now; that way we can write new functions later, using a different mechanism.

@Mte90
Copy link
Member Author

Mte90 commented Nov 18, 2024

So I removed the /../ now that stuff is internal, just the ending symbol is something that is not mandatory in the regex.
I agree to that in the future we can have different regex functions but right now is too much.

@Mte90 Mte90 requested a review from hdwalters November 19, 2024 09:32
@Mte90 Mte90 requested a review from Ph0enixKM November 19, 2024 09:33
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated
} else {
output = $ echo "{source}" | sed -e "{search}" $
}
if output == "" {
Copy link
Contributor

@hdwalters hdwalters Nov 19, 2024

Choose a reason for hiding this comment

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

See above for reasons why it's still recommended to use if output != "".

src/std/text.ab Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
src/tests/stdlib/contains_all.ab Show resolved Hide resolved
@Mte90 Mte90 requested a review from hdwalters November 20, 2024 10:22
src/std/text.ab Show resolved Hide resolved
src/std/fs.ab Outdated Show resolved Hide resolved
src/std/text.ab Outdated Show resolved Hide resolved
@Mte90 Mte90 requested a review from hdwalters November 25, 2024 10:20
Copy link
Contributor

@hdwalters hdwalters left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

Copy link
Member

@Ph0enixKM Ph0enixKM left a comment

Choose a reason for hiding this comment

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

Looking good

@Ph0enixKM Ph0enixKM merged commit a837380 into amber-lang:master Nov 30, 2024
1 check passed
lens0021 pushed a commit to lens0021/amber that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants