Skip to content
This repository has been archived by the owner. It is now read-only.

Contract requires string not null or empty #272

Merged
merged 2 commits into from
Feb 20, 2017
Merged

Contract requires string not null or empty #272

merged 2 commits into from
Feb 20, 2017

Conversation

ceddlyburge
Copy link
Contributor

Hi there

This pr fixes issue #209, and has tests.

I've added the relevant projects to the .2017 csproj files as well.

You seem to be ignoring my other pr, and I notice the build has been unstable for a long time and that there are a lot of skipped tests. Can I be any help? I don't have much spare time but I do get to geek a bit on my train journey to work most days.

Cheers

Cedd

@ceddlyburge
Copy link
Contributor Author

Actually it looks like I lied about the build being broken, and my other PR seems to have built ok now as well, so feel free to ignore me on that.

@siegfriedpammer siegfriedpammer modified the milestone: 4.10/5.2 Feb 20, 2017
@siegfriedpammer
Copy link
Member

You seem to be ignoring my other pr, and I notice the build has been unstable for a long time and that there are a lot of skipped tests.

Thanks for your contribution, sorry for the long wait. Yes, we had to figure out a threading issue.

Can I be any help? I don't have much spare time but I do get to geek a bit on my train journey to work most days.

Yes, there's a few things we'd need help with:

  • We'd love to have someone looking over the unit tests
  • Adjusting/extending existing code to properly support C# 7 (VS 2017)
  • Remove refactorings/analyzers (maybe turn them into Code fixes for built-in analyzers) that are already built into VS 2017

Important: Please open an issue once you start working on something, or comment on an issue, so everybody knows, what's already being taken care of.

If you want to have an extended discussion, please join us on gitter.

@siegfriedpammer siegfriedpammer merged commit 29bb3d7 into icsharpcode:master Feb 20, 2017
@ceddlyburge
Copy link
Contributor Author

ceddlyburge commented Mar 5, 2017 via email

@ceddlyburge
Copy link
Contributor Author

Hi Siegfried

I've finished my other project and now have time to work on this.

It sounds like you number one priority is "We'd love to have someone looking over the unit tests".

I have just checked out the latest code and there are a lot of skipped tests, so this seems like the place to start.

In order to avoid us all working on the same tests, shall I add an issue for some of them and then label myself on the issue?

If you know of any obvious groupings of tests that might all be fixed in the same way, then please let me know.

Thanks

Cedd

@christophwille
Copy link
Member

@siegfriedpammer or @Rpinski might chime in too, but: currently both are very time-constrained. So I'd say you have a free for all by default on those skipped tests.

@ceddlyburge
Copy link
Contributor Author

ceddlyburge commented Mar 29, 2017 via email

@ceddlyburge
Copy link
Contributor Author

Hi there

I created Issue #301 for a failing test a while ago, and then created a PR #304 to fix it.

This seems to have been ignored.

I was busy at the time so didn't get round to chasing it, but I have just looked in to it again.

Issue #304 (failing / skipped unit test) is still present in the current HEAD of master, and I have just merged your changes to my repo, where the issue is still fixed.

If you still want help in fixing up the tests, can you take a look at PR #304 and let me know what you think.

Thanks

Cedd

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

Successfully merging this pull request may close these issues.

3 participants