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

Restructuring to Eliminate "ChapterMain()" #16

Closed
10 tasks done
MarkMichaelis opened this issue Jul 19, 2017 · 6 comments
Closed
10 tasks done

Restructuring to Eliminate "ChapterMain()" #16

MarkMichaelis opened this issue Jul 19, 2017 · 6 comments
Assignees

Comments

@MarkMichaelis
Copy link
Member

MarkMichaelis commented Jul 19, 2017

Somewhere in the conversion, each listing was changed so that:

  • the listing files used ChapterMain rather than Main
  • Referenced a new project Called SharedCode rather than just linking to the necessary files in the Shared folder when required

At the time this was done (during project.json), I suspect it wasnt possible to have two static Mainmethods and be able to disambiguate which ones to use as the entry point via theStartupObject` property element. Going forward, this is not a satisfactory solution, however.

I would like to request a restructuring as follows:

  • - Remove the reference to the "SharedCode" project
  • - Delete the SharedCode project
  • - Add "linked" files that point to the Program.cs file in the Shared directory
  • - Add "liked" files that point to the other files in the Shared directory as needed (i.e. PI Calculator in the threading listings.
  • - Add the following to CSProj tiles to disambiguate which Main to use in startup.
    <PropertyGroup> <StartupObject>AddisonWesley.Michaelis.EssentialCSharp.Shared.Program</StartupObject> </PropertyGroup>
  • - Rename all ChapterMain methods to Main (consider doing this via a mass search and replace.
  • - Verify dotnet build and dotnet test work from the command line
  • - Verify dotnet test works from the command line.
  • - Verify build/test works from VS2017
  • - If a mass/automated search and replace is performed, please verify that all files stay recognizable to Git as UTF8 files.

It is acceptable if this is done one project at a time rather than with a PowerShell/batch file just as long as it is clearly recorded which projects are done and which need work.

Thoughts? Questions? Volunteers? :)

@MarkMichaelis MarkMichaelis added this to the v7.0: High Priority milestone Jul 19, 2017
@nlundby nlundby self-assigned this Jul 21, 2017
@nlundby
Copy link
Contributor

nlundby commented Jul 21, 2017

Everything should be completed as you requested. I ran the tests and get the same results pre vs post restructuring, so I'm hoping I put it back together correctly. I'm going to sleep on it before submitting the pull request, but its current state is on my fork https://github.com/nlundby/EssentialCSharp/tree/v7.0

@MarkMichaelis
Copy link
Member Author

Wow Neal... that was fast. Thanks!

Can you please identify what tests are failing for you. On Windows, all the tests are passing for me. Thanks!

@nlundby
Copy link
Contributor

nlundby commented Jul 24, 2017

Pull request submitted. These two tests were failing for me on both Mac and Windows:

Listing15.14.Tests.cs(31,57): error CS1501: No overload for method 'ExpectLike' takes 3 arguments [./EssentialCSharp/src/Chapter15.Tests/Chapter15.Tests.csproj]
Listing14.12.ProjectionWithSystem.Linq.Enumerable.Select.Tests.cs(40,36): error CS1501: No overload for method 'IsLike' takes 2 arguments [./src/Chapter14.Tests/Chapter14.Tests.csproj]

A fix was submitted in my pull request.

@MarkMichaelis
Copy link
Member Author

Is it possible that you haven't updated your sub modules so you have and old version of test too,s by chance? (Or, alternatively, I didn't update the sub module pointer to point to the most recent TestTools perhaps?)

@nlundby
Copy link
Contributor

nlundby commented Jul 24, 2017

Bingo! All is well. Yes, I believe the submodule pointer does not point to the most recent TestTools. I added some steps to the Readme to ensure the latest are downloaded. Unfortunately this did not resolve the mac(and now verified linux) test fail on listing08.16.

My 15.14 and 14.12 changes have been retracted in the pull request.

@COsborn2
Copy link
Contributor

COsborn2 commented Jul 8, 2019

From the comments and looking at the current state of the code it sounds like a PR was made against this and these changes are live.

@COsborn2 COsborn2 closed this as completed Jul 8, 2019
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

No branches or pull requests

3 participants