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.