Skip to content

Remove commented code #513

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

Closed
dan-drews opened this issue Jul 10, 2018 · 4 comments
Closed

Remove commented code #513

dan-drews opened this issue Jul 10, 2018 · 4 comments

Comments

@dan-drews
Copy link
Contributor

While digging into the source code, I discovered a large amount of commented out code throughout the solution.

Unless there is a reason behind this that I am not aware of, I think it is best to remove this. In a worst-case scenario, we have the git history to take care of this.

One example is in UnbufferedStream.cs, the following code is all commented out:

//}
//if (checkStream == null)
//{
//    checkStream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read);
//}
//byte[] cbuf = new byte[count];
//int checkRead = checkStream.Read(cbuf, 0, cbuf.Length);
//if (checkRead != read)
//{
//    Console.WriteLine("!!! bytes read mismatch at " + fileName + ": "  + checkStream.Position + " / " + Position + ": " +
//        "read = " + read + "; checkRead = " + checkRead);
//}
//else
//{
//    Console.WriteLine(">>> bytes read match at " + fileName + ": " + checkStream.Position + " / " + Position + ": " +
//        "read = " + read + "; checkRead = " + checkRead);
//}

I haven't yet discovered the extent of this, but I think some work should be done on this.

@shauheen
Copy link
Contributor

Thanks @dan-drews , this is great. I do not see a reason for keeping unnecessary commented out code.

@Ivanidzo4ka
Copy link
Contributor

Can we just nuke Microsoft.ML.InternalStreams?
Not sure how they get leaked in this repo.

@dan-drews
Copy link
Contributor Author

dan-drews commented Jul 11, 2018

@Ivanidzo4ka I was curious about that too. Is there any background on that project?

There is a lot of code in there that just feels very old. I came across several properties that were not auto-properties, there are tons of #IF compiler directives that seem to have no use, etc...

Update
I just went in and removed that project. Still able to clean/build. Should I go ahead and update the PR with this?

@Ivanidzo4ka
Copy link
Contributor

This is really old code which has nothing to do with ML.NET, and I'm not sure how it end up in this repository. Generally it's suppose to provide wrappers to access different types of streams (SQL tables for example) but again this is really old code, (pre dotnet CLR) which not suppose to be in this repository.
So feel free to delete whole project and any mentions of it.

Ivanidzo4ka pushed a commit that referenced this issue Jul 13, 2018
* First attempt at removing extra code comments

* Round #2

* Removing Microsoft.ML.InternalStreams per comment on #513

* Address notes from @Ivanidzo4ka

* Remove TreeOrderedCandidatesSearch

* Remove whitespace and reinstate commented out tests
eerhardt pushed a commit to eerhardt/machinelearning that referenced this issue Jul 27, 2018
…#514)

* First attempt at removing extra code comments

* Round #2

* Removing Microsoft.ML.InternalStreams per comment on dotnet#513

* Address notes from @Ivanidzo4ka

* Remove TreeOrderedCandidatesSearch

* Remove whitespace and reinstate commented out tests
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants