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

Fix #1283 (Spaces in filename cause ReleaseEntry Regex to fail) #1401

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cbenard
Copy link
Contributor

@cbenard cbenard commented Oct 24, 2018

Regex change allows spaces in the filename for while requiring at least one non-space character to fix #1283. Worked with @JohnThomson to refined the regex in the issue comments.

@JohnThomson
Copy link
Contributor

There are extensive unit tests for this function...could you figure out a new one that fails without the fix?

@cbenard
Copy link
Contributor Author

cbenard commented Oct 24, 2018

@JohnThomson, I didn't have a dev env set up for this. I went ahead and cloned in the NuGet submodule and opened it in VS2017. Even in master, there are a ton of failing tests for me. Also, I looked at the tests for UpdateManager and it's not pleasant. It's actually copying files out of the test\fixtures folder and there aren't any that have spaces.

I don't want to try to add files with spaces to the repo and given that there are failing tests already, this is beyond the time I have to spend on it. I'm confident in the RegEx that we worked on together. If you'd like to mess with the tests, please do your own PR. If the tests were all green already, I would have likely proceeded.

image

@JohnThomson
Copy link
Contributor

I think I've seen problems running all the tests, too. But there's a suite of tests, the second last entry in your screen shot, for ReleaseEntryTests, and many of them test this specific method, passing it a string (the file contents). So I believe it should be fairly easy to add a test that requires the fix without adding any files.
I'd be willing to do that if Paul wants it...not just in case he might, though.

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.

Spaces in filename cause ReleaseEntry Regex to fail
2 participants