Skip to content

Conversation

OnesQuared
Copy link
Contributor

The latest commit should have most of the project, apologizes for making multiple commits prior. Hopefully I didn't screw anything up. Let me know if any changes are needed

@ThomasJayWilliams ThomasJayWilliams self-assigned this Apr 9, 2019
Copy link
Owner

@ThomasJayWilliams ThomasJayWilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all - you should rename back the project names you changed to SDML. Projects should be named in upper case.

Next, you forgot to rename some files, especially in SDML.NET.Core. All of the files have old names. I don't know what IDE you use, but Visual Studio 2017 has some beatiful functions to rename anything. For example, when you rename class file Visual Studio propose you to change class name inside this file and rename all references on this class.

Also, you didn't rename interfaces. They should be renamed too.
Basically, everything in solution, that has SDML in the name should be renamed to Sdml, except projects.

@ThomasJayWilliams ThomasJayWilliams added code review failed Issue requires additional changes improvement Existing code improvement labels Apr 9, 2019
@ThomasJayWilliams ThomasJayWilliams added this to the Initial stable build milestone Apr 9, 2019
@OnesQuared
Copy link
Contributor Author

I'm using visual studio code for this at the moment, but it seems the file names didn't change even though I did so in the code, maybe I'll switch over to visual studio. Not sure how the project name changed, but I'll change it back to the capitals.

@ThomasJayWilliams
Copy link
Owner

Sweet.

By the way, the fact you're using Visual Code explains something - the solution doesn't build. You should always build solution to be sure that solution is working and nothing broken. It is crucial to do not push into repository broken project.

Anyway, I guess I'll check request tomorrow, if you'll finish.

@OnesQuared
Copy link
Contributor Author

Alright, I'll try and figure things out with studio since code seems to be a bit of a problem. I'll see what I can do tonight, if not tomorrow afternoon probably.

@ThomasJayWilliams
Copy link
Owner

ThomasJayWilliams commented Apr 9, 2019

Sounds awesome, thanks for spending time on this project.

Oh, I almost forgot - if you will continue working on issue change the label to in progress. This may seem a bit annoying, but in enterprise projects you will forced to do things like this. So it's a lot easier to form a habit now.

@OnesQuared OnesQuared added the in progress Issue is currently in progress label Apr 9, 2019
@OnesQuared
Copy link
Contributor Author

So according to This, there is problem with lower and upper case in file names. So I made a new pull request by forking the repository and reuploading all the name class and file name changes to apply. Not sure if it will actually work, but trying to merge into the issue #28 branch to see if it works or not

@ThomasJayWilliams ThomasJayWilliams removed the in progress Issue is currently in progress label Apr 10, 2019
@ThomasJayWilliams ThomasJayWilliams added cancelled Issue was cancelled and removed code review failed Issue requires additional changes labels Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cancelled Issue was cancelled improvement Existing code improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants