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 a function to add backslash before a space #541

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

MichalKesl
Copy link
Contributor

@MichalKesl MichalKesl commented Aug 10, 2022


If path has spaces the function will modify the name and add backslash before the space. a local test was preformed, the solution works.

fixes #535

@rwest
Copy link
Member

rwest commented Aug 10, 2022

If you do want to just replace all the with \ then you could consider folder_name.replace(' ','\ ') instead of your split and recombine routine.

In [3]: folder_name = 'foo bar  baz'

In [4]: folder_name.replace(' ','\ ')
Out[4]: 'foo\\ bar\\ \\ baz'

But there may be a better fix. This won't escape anything except spaces.

Most functions don't need the path escaping. The few that do include os.system(), but it might be safer to replace those calls with subprocess and then you avoid the need. But maybe it's for doing something over SSH where shell escaping is not avoidable. In that case, consider shlex.quote()

Return a shell-escaped version of the string s. The returned value is a string that can safely be used as one token in a shell command line, for cases where you cannot use a list.

Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks, please see some comments

arc/common.py Outdated
arc_r_symbols = [atom.element.symbol for atom in
chain(*tuple(arc_reaction.r_species[i].mol.atoms for i in range(num_rs)))]
arc_p_symbols = [atom.element.symbol for atom in
chain(*tuple(arc_reaction.p_species[i].mol.atoms for i in range(num_ps)))]
Copy link
Member

Choose a reason for hiding this comment

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

All the modifications to the file up to this point are style changes. If you think they are necessary, please add them as a different commit. I would recommend removing them, the previous version is "OK", I think

arc/common.py Outdated
new_folder_name (str): modified folder name.

"""
folder_name.split(" ")
Copy link
Member

Choose a reason for hiding this comment

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

this line has no effect (we don't store the returned value)

arc/common.py Outdated
"""
folder_name.split(" ")
new_folder_name = ""
for i in folder_name.split(" "):
Copy link
Member

Choose a reason for hiding this comment

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

minor: I would recommend changing i to a different name, i is usually an integer iterator. maybe use split?

Copy link
Member

Choose a reason for hiding this comment

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

You can do either folder_name.split() or folder_name.split(" "), they are the same (the first one is shorter or "cleaner")

arc/common.py Outdated
folder_name.split(" ")
new_folder_name = ""
for i in folder_name.split(" "):
new_folder_name += i + "\ "
Copy link
Member

Choose a reason for hiding this comment

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

If you'd like, you can achieve this in a single line instead of the loop. Try:
new_folder_name = '\ '.join(folder_name.split())

@MichalKesl MichalKesl changed the title Issue #535 solved Added a function to add backslash before a space May 29, 2023
@codecov
Copy link

codecov bot commented May 29, 2023

Codecov Report

Merging #541 (df9dff7) into main (c440aab) will increase coverage by 0.01%.
Report is 3 commits behind head on main.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #541      +/-   ##
==========================================
+ Coverage   73.18%   73.20%   +0.01%     
==========================================
  Files          99       99              
  Lines       26522    26534      +12     
  Branches     5546     5546              
==========================================
+ Hits        19409    19423      +14     
+ Misses       5772     5771       -1     
+ Partials     1341     1340       -1     
Flag Coverage Δ
unittests 73.20% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
arc/common.py 82.60% <100.00%> (+0.33%) ⬆️
arc/common_test.py 99.85% <100.00%> (+<0.01%) ⬆️
arc/main.py 51.03% <100.00%> (+0.07%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@MichalKesl MichalKesl requested a review from alongd May 29, 2023 11:43
Copy link
Member

@alongd alongd left a comment

Choose a reason for hiding this comment

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

Thanks @MichalKesl, looks good!
I added some comments, please take a look

arc/common_test.py Outdated Show resolved Hide resolved
self.assertTrue(common.fill_in_the_blanks(ex1), "michalkfir")
self.assertTrue(common.fill_in_the_blanks(ex2), "michal\\ kfir")
self.assertTrue(common.fill_in_the_blanks(ex3), "mich\\ al\\ kfir")
self.assertTrue(common.fill_in_the_blanks(ex4), "michal\\ \\ kfir")
Copy link
Member

Choose a reason for hiding this comment

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

why do we have two slashes in the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do michal kfir.replace(" ", "\ ") the string changes to michal\\ kfir, because \ is an operator (like \n).

@alongd
Copy link
Member

alongd commented Aug 19, 2023

@MichalKesl, when you get a chance, can we finish this PR?

@calvinp0
Copy link
Member

@MichalKesl is this PR relevant?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running a job when the path contains spaces crushes.
4 participants