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

use filepath.Join to concat path; fix loop err #21

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
5 participants
@zyfdegh
Copy link
Contributor

commented May 29, 2017

Issue #20

@shroomist

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2017

looks alright, merge? @Waldz

@Waldz
Copy link
Member

left a comment

From code I dont see loop happening.
Need test which reproduces this error.

@@ -15,8 +15,8 @@ const finenameCharset = "1234567890abcdefghijklmnopqrstuvwxyz"
func tempFilename(directory, filePrefix, fileExtension string) (filePath string) {
for i := 0; i < 10000; i++ {
filePath = directory + "/" + filePrefix + randomString(10, finenameCharset) + fileExtension
if _, err := os.Stat(filePath); os.IsExist(err) {
continue
if _, err := os.Stat(filePath); os.IsNotExist(err) {

This comment has been minimized.

Copy link
@Waldz

Waldz Nov 28, 2017

Member

Need test for this. How to reproduce this loop error?

This comment has been minimized.

Copy link
@donce

donce Jan 4, 2018

Contributor

@Waldz currently, this loop is happening 10k times, no matter whether we find existing files or not (try to simulate the execution of lines).

This class does not contain any tests and it's coupled to OS, so I think it's ok for this fix to be merged without tests, because it's very critical!

@donce

donce approved these changes Jan 4, 2018

Copy link
Contributor

left a comment

This looks quite critical and should be merged IMHO - we're generating 10k randoms for each temp file.

@tadovas

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

LGTM

@donce

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

This could be merged without tests, they will be added here MYST-219.

@Waldz

Waldz approved these changes Jan 5, 2018

@@ -15,8 +15,8 @@ const finenameCharset = "1234567890abcdefghijklmnopqrstuvwxyz"
func tempFilename(directory, filePrefix, fileExtension string) (filePath string) {
for i := 0; i < 10000; i++ {
filePath = directory + "/" + filePrefix + randomString(10, finenameCharset) + fileExtension

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 5, 2018

Member

Should use filepath.Join() to work on different OSes

@Waldz Waldz merged commit 61e4186 into mysteriumnetwork:master Jan 5, 2018

@Waldz

This comment has been minimized.

Copy link
Member

commented Jan 5, 2018

Thanks @zyfdegh, be welcome to our squad ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.