-
Notifications
You must be signed in to change notification settings - Fork 103
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
Break Method Convert #816
Comments
Could you add unit tests at same time that is moved? |
Work all at once would only cause two situations, either very large pr again, or very surface unit tests. Unit tests for this has being missing for years, adding them should be systemically and carefully designed and should be another separate task. Just checked again, unit tests for these are actually hard, will take a while for me to figure out how to add them. |
You say that splitting this method into several files is necessary to maintain it better and improve the coverage, I think that if you split it up it has to be to add that coverage, otherwise, we don't improve anything. In addition, this way we can ensure that this code is correct. |
@shargon step by step. How about we focus on fixing issues? Large file is hard to follow and maintain, how about we break it into smaller files step by step, then we work on checking C# language specification to find what is missing, then unit tests to ensure everything works fine, and then comments to make sure others can understand? |
Will directly work on the original methodconvert file |
As is in #813, the methodconvert file is too large to maintain, we need to break it into smaller files.
This issue will trace the breaking prs. Breaking process plan:
The text was updated successfully, but these errors were encountered: