-
-
Notifications
You must be signed in to change notification settings - Fork 248
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
Walk runners dir #416
Walk runners dir #416
Conversation
Hi @brunomgalmeida thanks for the PR! I think this is a nice feature Would you mind adding a test for it, please? In tests.config_service_test.ConfigServiceTest |
thanks @bugy Would you be able to help me writing the test for this one, and I should be able to write tests from here onwards. How can I run the tests? |
Hi @brunomgalmeida you could run the tests from Regarding the test, you might copy
|
src/config/config_service.py
Outdated
# Read config file from within directories too | ||
for _root, _dirs, _files in os.walk(configs_dir, topdown=True): | ||
for name in _files: | ||
files.append( re.sub ( r'^.*/runners/', '', os.path.join( _root, name) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just append name
here. It doesn't include parent path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, simply adding the name returns different results and causes the tests to fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, true. I just wanted to avoid having forward slashes for windows compatibility.
In this case I would suggest to files.append(os.path.join(_root, name))
And later, in the line 218, don't do os.path.join(configs_dir, config_path)
and use config_path directly
PS could you change the formatting rules please, and remove whitespaces in parentheses? E.g. ( a, b, c )
=> (a, b, c)
It would be better to have the same style across the project, if possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for changes, I have only 1 comment, concerning forward slashes in the path
src/config/config_service.py
Outdated
# Read config file from within directories too | ||
for _root, _dirs, _files in os.walk(configs_dir, topdown=True): | ||
for name in _files: | ||
files.append( re.sub ( r'^.*/runners/', '', os.path.join( _root, name) ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, true. I just wanted to avoid having forward slashes for windows compatibility.
In this case I would suggest to files.append(os.path.join(_root, name))
And later, in the line 218, don't do os.path.join(configs_dir, config_path)
and use config_path directly
PS could you change the formatting rules please, and remove whitespaces in parentheses? E.g. ( a, b, c )
=> (a, b, c)
It would be better to have the same style across the project, if possible
files.append(os.path.join(_root, name)) is that the path returned includes the config+runners directory
Hence the funky regular expression to remove it. If your only concern is the Windows paths we can update the regex to files.append( re.sub ( r'^.*(/|\\)runners(/|\\)', '', os.path.join( _root, name) ) )
|
Let me check it later today |
# Conflicts: # src/config/config_service.py
Merged to master |
This provides a better way to organize things when we have many scripts and runners