Skip to content
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

Remove grocer's apostrophes #2546

Merged

Conversation

TheNetworkIsDown
Copy link
Contributor

Copy link
Member

@netniV netniV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of these changes are in comments so have zero effect on the code (though I have made a couple of suggestions). The only code-related issue that could potentially be a problem is the changing of the name in the host_cpu.xml files in the script_query/script_server directories.

Thanks for the review and getting the language correct 👍

lib/rrd.php Outdated
@@ -1482,7 +1482,7 @@ function rrdtool_function_graph($local_graph_id, $rra_id, $graph_data_array, $rr
$graph_items[$j]['vdef_cache'] = $vdef;
}

/* +++++++++++++++++++++++ LEGEND: TEXT SUBSTITUTION (<>'s) +++++++++++++++++++++++ */
/* +++++++++++++++++++++++ LEGEND: TEXT SUBSTITUTION ("<>"s) +++++++++++++++++++++++ */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you change lines very similar to this one so that they do not include the 's' suffix? It really isn't necessary since it is more about the text it is replacing that the plural nature of them that may be present (plus it's only a code comment)

lib/rrd.php Outdated

/* +++++++++++++++++++++++ GRAPH ITEMS: CDEF's +++++++++++++++++++++++ */
/* +++++++++++++++++++++++ GRAPH ITEMS: CDEFs +++++++++++++++++++++++ */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an addendum to the previos comment I made, the 's' suffix on lines like this are fine where they refer to a name not piece of text to be replaced.

Also make sure to replace all of the fancy \'s at the end of the line,
but make sure not to get rid of the "\n"'s that are supposed to be
Also make sure to replace all of the fancy "\"s at the end of the line,
but make sure not to get rid of the "\n"s that are supposed to be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are editing the comment, can you remove the word "fancy" as it seems redundant, and maybe change the "\"s to backslashes (\) and do the same with the newline line part too.

@@ -1,5 +1,5 @@
<interface>
<name>Get Host MIB CPU's</name>
<name>Get Host MIB CPUs</name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this will cause a new input method to appear, I don't think it should be an issue but @cigamit would know better.

@@ -1,5 +1,5 @@
<interface>
<name>Get Host MIB CPU's</name>
<name>Get Host MIB CPUs</name>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this will cause a new input method to appear, I don't think it should be an issue but @cigamit would know better.

@netniV netniV requested a review from cigamit March 20, 2019 10:32
@netniV netniV merged commit c3195c1 into Cacti:develop Mar 22, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants