-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
Hide private enums from documentation #84396
Conversation
4139a51
to
7209303
Compare
I think it is working properly |
3ba8cf4
to
447030e
Compare
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 tested this PR and it worked correctly.
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.
Tested myself and works as intended.
if (cd.enums.has(e)) { | ||
if (cd.enums[e].is_deprecated) { | ||
DEPRECATED_DOC_TAG; | ||
} | ||
|
||
if (cd.enums[e].is_experimental) { | ||
EXPERIMENTAL_DOC_TAG; | ||
} | ||
if (cd.enums[key].is_deprecated) { | ||
DEPRECATED_DOC_TAG; | ||
} | ||
if (cd.enums[key].is_experimental) { | ||
EXPERIMENTAL_DOC_TAG; |
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.
As far as I understand, it is not guaranteed that cd.enums
has key
. Above:
if (cd.enums.has(key)) {
if (cd.enums[key].description.strip_edges().is_empty() && E.key.begins_with("_")) {
continue;
}
}
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.
So should we still be checking if cd.enums.has(key)
or should we skip any enum that doesn't have a key?
Edit: if I understand it correctly cd.enums
will always have key
editor/editor_help.cpp
Outdated
key = key.get_slice(".", 1); | ||
} | ||
if (cd.enums.has(key)) { | ||
if (cd.enums[key].description.strip_edges().is_empty() && E.key.begins_with("_")) { |
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.
Above the enums are filtered on script docs, but not here, is that an oversight?
if (cd.enums[key].description.strip_edges().is_empty() && E.key.begins_with("_")) { | |
if (cd.is_script_doc && cd.enums[key].description.strip_edges().is_empty() && E.key.begins_with("_")) { |
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.
correct me if I'm wrong but we shouldn't have to check cd.is_script_doc every time because it doesn't change per enum.
the reason we are checking if the description is empty or if the name starts with "_" is it is possible for enums that don't need documentation generated to be in the for loop.
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.
Then there should be some other flag to control this as the filtering is on script docs only
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.
oh I didn't realize we only filter on script docs.
editor/editor_help.cpp
Outdated
bool has_enums = false; | ||
if (enums.size()) { | ||
for (KeyValue<String, DocData::EnumDoc> &E : cd.enums) { | ||
if (cd.is_script_doc && cd.is_script_doc && E.key.begins_with("_") && E.value.description.strip_edges().is_empty()) { |
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.
bool has_enums = false; | |
if (enums.size()) { | |
for (KeyValue<String, DocData::EnumDoc> &E : cd.enums) { | |
if (cd.is_script_doc && cd.is_script_doc && E.key.begins_with("_") && E.value.description.strip_edges().is_empty()) { | |
bool has_enums = enums.size() && !cd.is_script_doc; | |
if (enums.size() && !has_enums) { | |
for (KeyValue<String, DocData::EnumDoc> &E : cd.enums) { | |
if (E.key.begins_with("_") && E.value.description.strip_edges().is_empty()) { |
No need to check these every time, the loop accepts enums if they're non-script doc ones, so we can skip this part
Might be a more elegant way to do this just a suggestion, but the loop is unnecessary if it's not a script doc
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.
yeah you're right.
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.
LGTM, would need to verify the comment about the enum key though, I'd suggest keeping it as it was to avoid any weirdness
Should I go ahead and combine the commits? |
Looks Good To Me :) Yes please squash |
whenever I do git rebase it says there are 1382 commits to push. this has happened before and it didn't mess anything up but it did send a message in every pr that was in it. Is there any way to fix it? |
It's just because you're updating your branch, the others should be just the normal ones, just that the branch doesn't contain them upstream |
c72945f
to
5b4216f
Compare
Squashed!! |
No problem at all! It's a learning experience :) |
Apologies for yet another nitpick, but could you amend the commit message to be a bit more explicit (and ideally start with a capital letter like other commits)? The title of this PR would be a good option, for a changelog reader I think the "from documentation" part is important to get the context. I can do it myself too if needed. |
5b4216f
to
a5a8f0a
Compare
Done |
Thanks! |
This loops through all the enums and checks if it has a description (doc comment) or if it is public. it also renames the variable
e
to a more descriptive namefixes #84119