Skip to content

Conversation

@dlsniper
Copy link
Member

Technically only directory run configs have this issue (package running doesn't support specifying ... yet, I'll do a different PR for that if it's deemed of interested, so far no such request tho).

Copy link
Contributor

Choose a reason for hiding this comment

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

do not compare objects with ==

Copy link
Contributor

Choose a reason for hiding this comment

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

write a todo to remove this as soon as multiple packages coverage will be supported with the link to corresponding issue

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually it is quite bad and inconsistent solution, it won't work good if testing directory is not equal to working directory

@dlsniper
Copy link
Member Author

Updated. Thank you.

@zolotov
Copy link
Contributor

zolotov commented Jan 21, 2016

Actually it is quite bad and inconsistent solution, it won't work good if testing directory is not equal to working directory

Still actual

@dlsniper
Copy link
Member Author

I've totally missed that notification, sorry.

@dlsniper
Copy link
Member Author

Updated and improved (hopefully).

@zolotov
Copy link
Contributor

zolotov commented Jan 21, 2016

Looks good, thanks. But now it won't work if testing directory is outside of working directory :)

@dlsniper
Copy link
Member Author

But now it won't work if testing directory is outside of working directory :)

I've tested with the working directory being a parent and even a different leaf of the same parent and they worked both (second one needed "convincing" but still worked). Can you please provide a sample of what's not working? Thank you.

@zolotov
Copy link
Contributor

zolotov commented Jan 22, 2016

@dlsniper empty dir path, empty working dir path, directories on different drive – anything that lead to null relative path leads to incorrect work of your patch

@dlsniper
Copy link
Member Author

Updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now flow is overcomplicated. Path can end with separator only if relativePath is not null and appending it with File.separator makes sense only if it's not coverage run. As a result you make 3 ifs instead 2.

The only thing you change is path suffix ("..." or "."), you can extract it in variable and append it to executor for both cases (with null relativePath and notnull relativePath). It requires only 1 new line and simple modification of 2 other lines, without any extra ifs, adding separators, etc.

@dlsniper
Copy link
Member Author

Done. It does indeed look much better.

zolotov added a commit that referenced this pull request Jan 22, 2016
Improve coverage support for packages (fixes #2276)
@zolotov zolotov merged commit faae912 into go-lang-plugin-org:master Jan 22, 2016
@dlsniper dlsniper deleted the improve-coverage-support branch January 22, 2016 19:35
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.

2 participants