-
Notifications
You must be signed in to change notification settings - Fork 756
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
Clean up warnings in DnnExportImport #4373
Conversation
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one typo and a couple of interrogations, otherwise, happy with this.
DNN Platform/Modules/DnnExportImport/Components/Controllers/BaseController.cs
Outdated
Show resolved
Hide resolved
file.EndDate ?? Null.NullDate, | ||
file.EnablePublishPeriod, | ||
Null.NullInteger); | ||
////file.ContentItemId ?? Null.NullInteger);--If we keep it we will see FK_PK relationship errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we just delete this line ?
localTab.IsVisible = otherTab.IsVisible; | ||
EntitiesController.Instance.SetTabSpecificData(localTab.TabID, false, localTab.IsVisible); | ||
|
||
// tabController.UpdateTab(localTab); // to clear cache |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove commented out code ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, doesn't matter much to me either way. For both of them you've called out here, there's at least a comment indicating the thinking somewhat, which might protect against re-making the same mistake.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, makes sense
594 obsolete and documentation warnings remain Co-authored-by: Daniel Valadas <info@danielvaladas.com>
9512135
to
2b5853c
Compare
594 obsolete and documentation warnings remain