Changesets - some comments

I don't want to be an anal nit-pick, but I've been getting familiar with the code by looking at the changes in recent changesets...

1) Not that I've looked at all the codes & changesets, but generally I've not seen previously commented out code that is followed with the replacement - usually the code is just replaced.  Since we have source control, we can always get back to old code.  I suspect it's just that the developer is not sure the change will work, and then just does not clean up after testing.  It is a bit distracting though.  E.g.

src/Libraries/Nop.Services/Catalog/ProductService.cs

            //sort products
            if (orderBy == ProductSortingEnum.Position && categoryId > 0)
            {
                //category position
                //query = query.OrderBy(p => p.ProductCategories.FirstOrDefault().DisplayOrder);

                //category position
                query = query.OrderBy(p => p.ProductCategories.Where(pc => pc.CategoryId == categoryId).FirstOrDefault().DisplayOrder);
            }
            else if (orderBy == ProductSortingEnum.Position && manufacturerId > 0)
            {
                //manufacturer position
                //query = query.OrderBy(p => p.ProductManufacturers.FirstOrDefault().DisplayOrder);

                //manufacturer position
                query = query.OrderBy(p => p.ProductManufacturers.Where(pm => pm.ManufacturerId == manufacturerId).FirstOrDefault().DisplayOrder);
            }


src/Presentation/Nop.Web.Framework/DependencyRegistrar.cs

            //builder.Register(x => (IEfDataProvider)x.Resolve<BaseDataProviderManager>().LoadDataProvider()).As<IDataProvider>().InstancePerDependency();
            builder.Register(x => (IEfDataProvider)x.Resolve<BaseDataProviderManager>().LoadDataProvider()).As<IEfDataProvider>().InstancePerDependency();




2) Sometimes the comments are not updated, or there are typos
e.g  changed the time to 10 min (60 * 10), but comment still says "run each hour: "

src/Presentation/Nop.Web/Web.config

       <Thread seconds="60">
         <task name="Emails" type="Nop.Services.Messages.QueuedMessagesSendTask, Nop.Services" enabled="true" stopOnError="false" maxTries="5" />
       </Thread>
+    <!--run each hour: 60*10=600-->
+    <Thread seconds="600">
+      <task name="DeleteGuestsTask" type="Nop.Services.Customers.DeleteGuestsTask, Nop.Services" enabled="true" stopOnError="false" olderThanMinutes="1440" />
+    </Thread>
       <!--run each 15 minutes: 60*15=900-->
       <Thread seconds="900">


3) The change set for "typo fixed", where all the Notes.txt files were updated

-We know that they're referenced by them main web applciations. So there's no need to delpoy them.
+We know that they're referenced by them main web applciations. So there's no need to deploy them.

Looks like there's another typo on that same line "THEM"  -->  "them main web ..."  - I assume should be "THE"  -->  "the main web ..."


4) The change set for "New feature. Products can require that other products are added to the cart (Product X requires Product Y).

I also see Blog related stuff in this changeset : BlogExtensions, BlogPots,
Are these really related to "products can require that other products"?
We developers would certainly appreciate changesets being feature/change specific, for better understandability and learning :)

Sorry to be so picky.  I hope these suggestions are useful.
Thanks.

New York wrote:

I agree that good commit messages are helpful and lots of commented out code hurts the eyes, sometimes a refactor touches more than one part of the code. If you've ever used efficiency tools like ReSharper you know how a refactor can stretch across many files and some are seemingly unrelated.

In addition to these refactors sometimes it is a bit overkill for us to checkin a change on every file if we simply went through fixing typos, comments, or very minor changes like HTML.

Your suggestions are good ones, but they can't always be implemented without a cost.