search

OutpostUniverse / OP2Utility

C++ library for working with Outpost 2 related files and tasks.
MIT License
4 stars 0 forks source link

Add global namespace OP2Utility #383

Closed Brett208 closed 3 years ago

Brett208 commented 3 years ago

Closes #237

Brett208 commented 3 years ago

I didn't think about all the extra tabs causing so much noise in the PR...

DanRStevens commented 3 years ago

Ahh, as soon as I read the title, I realized this was going to have merge conflicts with the other two branches that were recently merged.

DanRStevens commented 3 years ago

If you change nothing but the tabs, the diff for that commit is usually pretty easy to read. Though if you change so much as one other character, the diff tends to be extremely messy and hard to read.

In the interest of keeping diffs easy to read and review, sometimes I'll make an intermediate commit with a code change, but no corresponding indentation change. For instance, adding a namespace wrapper block, but not adding corresponding indentation. That means there will be a commit with bad indentation, though I think this is acceptable, as it's temporary in one commit, generally fixed in the following commit, and can make review much easier due to the simpler diffs.

Brett208 commented 3 years ago

@DanRStevens, If you would prefer, I am ok redoing this branch trying to separate the indentation change from the actual insertion of code change. If you do not prefer the rework, I will just fix the merge conflict. It is probably about 30 minutes for me to redo in a new branch which isn't a big deal assuming I can get Visual Studio's auto complete functions to assist me instead of hindering me...

Regarding the comment about not mixing template and non-template functions, I had not heard of this guideline but I am admittedly weak with templates. What are your thoughts about changing the name of the template function? I'm not experienced or picky about it either way. Could just name it DirInternal or DirTemplate.

I wanted to complete the namespace addition and a bit of work with the organization of the source code files before switching towards work in OP2BitmapConverter.

DanRStevens commented 3 years ago

I was kind of thinking renaming the template function might be a good idea.

Not really a strong preference for rework versus patch up. Maybe a slight preference towards rework if other changes are going to be made. I suppose it would be easier to verify nothing goes badly trying to fix merge conflicts if there was some rework instead.

Downstream projects will need to be updated after this change is merged. Actually, as it stands, downstream projects might already need to be updated due to the StringHelper rename.