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

Added 'rglob' function to stdlib. #242

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Added 'rglob' function to stdlib. #242

wants to merge 2 commits into from

Conversation

vitorarins
Copy link
Member

No description provided.

@codecov-io
Copy link

Codecov Report

Merging #242 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
- Coverage   56.96%   56.94%   -0.02%     
==========================================
  Files          26       25       -1     
  Lines        4403     4401       -2     
==========================================
- Hits         2508     2506       -2     
  Misses       1653     1653              
  Partials      242      242
Impacted Files Coverage Δ
internal/sh/util.go 75% <0%> (-4.55%) ⬇️
internal/sh/rfork.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 556fbc5...78de908. Read the comment docs.

Copy link
Collaborator

@i4ki i4ki left a comment

Choose a reason for hiding this comment

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

overall code is nice. If this test approach standardize, we can make a nice lib to easy other tests.

Maybe we could add this function to a proper 'module' directory, maybe called 'filepath' (like Go) or 'path' ?

Then, using it will be something like:

import filepath/rglob

results <= rglob("*")

We need to always make sure the script do not abort in stdlib functions.

@@ -0,0 +1,29 @@
fn dir_glob(dir, pattern, results) {
files <= find $dir -maxdepth 1 -mindepth 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

stdlib functions should never abort the script. We must treat (or ignore in some cases) all errors.

@@ -0,0 +1,29 @@
fn dir_glob(dir, pattern, results) {
files <= find $dir -maxdepth 1 -mindepth 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can optimize this a lot passing -type d to get only the directories.

files <= split($files, "\n")

for f in $files {
_, status <= test -d $f
Copy link
Collaborator

Choose a reason for hiding this comment

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

if asking only directories to find then this check could be skipped.
What do you think?

}

fn rglob(pattern) {
working_dir <= pwd
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think this is the right way to handle the rglob. It will only work in the current directory? I'm not saying we must add support to the '/path/**' syntax, but maybe the signature could be rglob(dir, pattern).
What do you guys think?

_, _ <= mkdir $hello_dir
_, _ <= mkdir $world_dir

honda_txt = $hello_dir+"/honda.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

kkkkkkk

mkdir $hello_dir
mkdir $world_dir

honda_txt = $hello_dir+"/honda.txt"
Copy link
Collaborator

Choose a reason for hiding this comment

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

kkkkk

@i4ki
Copy link
Collaborator

i4ki commented Oct 14, 2017

@vitorarins
https://github.com/DoctorWkt/unix-jun72/blob/master/src/cmd/glob.c

Version 1 of unix had a program called glob, very useful =) Maybe we create a tool like that in stdbin to be used in conjunction with our rglob function.

@katcipis
Copy link
Member

katcipis commented Oct 16, 2017

@tiago4orion the idea is to remove the builtin glob function then ?

@vitorarins
Copy link
Member Author

vitorarins commented Oct 16, 2017

Maybe we create a tool like that in stdbin...

Awesome!

@katcipis @tiago4orion What do you think about creating a new glob that is capable of accepting the two asterisks (**) syntax?

Copy link
Collaborator

@i4ki i4ki left a comment

Choose a reason for hiding this comment

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

added something I found when running the rglob tests

import rglob

fn setup() {
temp_dir <= mktemp -d
Copy link
Collaborator

Choose a reason for hiding this comment

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

this directory is leaking.

touch $civic_txt
touch $civic_sh

return $temp_dir
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to return the cleaning function also, something like:

       fn clean() {
	    rm -rf $temp_dir
        }

	return $temp_dir, $clean

@i4ki
Copy link
Collaborator

i4ki commented Oct 17, 2017

@katcipis I don't think so. I prefer a builtin function that handles ** also, or a function in stdlib.. I sent the link to the unix glob because maybe we can create a helper binary to help the rglob nash function. But maybe this doesn't make sense.

@katcipis
Copy link
Member

@tiago4orion If the builtin function will continue to exist I see very little incentive to have a helper binary...not sure in what will exactly it help if we give support on the glob function.

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.

4 participants