Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Optimize IndexingContentHandlers perf #13721
Optimize IndexingContentHandlers perf #13721
Changes from 7 commits
6301402
a31de04
78032d1
26eb712
383704c
d2fbdcb
d3944c0
88ac104
c66e3fa
1628549
7a5b3e2
bf927de
e32983f
e46cb6c
9708f08
cf2a4d0
4f4e10e
66c2068
0c5bfa2
20c966e
ec3d6c7
dfcd821
26acf51
3d4d968
5fc7c5b
876d976
98e694e
5fe344e
f2bfa57
36c7674
eb7ab7c
759c477
11c76ad
c0075be
1af75f6
28e9928
a894ed5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
why nullable
bool?
?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.
I will take a look at these comments @sebastienros
I just wanted to have an optional param there so that we don't need to pass it every time.
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.
The new
IsPublishing
worries me, the DefaultContentManager mentions that something can be null and handlers should take that into account, not create new dynamic fields like this. It's like saying we have a thing (Publish) but it's not really for that (Publishing)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.
And by that I mean on the DefaultContentManager.
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.
Though, I did not change the behavior of what it was doing before. So, I'm affecting the PublishingItem only when we are actually publishing. I'm just moving it to a param instead of leaving it as a public setter. Because, that's what we don't want, to let people affect this PublishingItem other than on instanciation.
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.
That or we should have an Unpublishing event