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

Add a doc comment to StringReplace #748

Merged
merged 3 commits into from
Dec 4, 2023
Merged

Conversation

GeckoEidechse
Copy link
Member

This is an initial test to get formatting etc agreed upon to then build on this further

Related to #664

This is an initial test to get formatting etc agreed upon to then
build on this further
@GeckoEidechse GeckoEidechse added the feedback wanted Feedback is wanted whether the changes by this PR are desired label Oct 17, 2023
@GeckoEidechse GeckoEidechse requested a review from F1F7Y October 17, 2023 15:17
@GeckoEidechse
Copy link
Member Author

GeckoEidechse commented Oct 17, 2023

So looking at this in the diff rn, I think we should drop the ///--------- decorators before/after doc comment cause it just adds more (unnecessary lines).

So like

...
string function GetMapDisplayDesc( string mapname )
{
	return "#" + mapname + "_CLASSIC_DESC"
}

/// Sends a string message to player
/// * `baseString` - The input string to search through
/// * `searchString` - Find this substring...
/// * `replaceString` - ...and replace with this substring
/// * `replaceAll` - Whether to replace all occurences or just the first
/// * `caseInsensitive` - Whether to consider casing (upper/lower)
///
/// Returns the updated string
string function StringReplace( string baseString, string searchString, string replaceString, bool replaceAll = false, bool caseInsensitive = false )
{
	bool loopedOnce = false
	string source = caseInsensitive ? baseString.tolower() : baseString
	var findResult = source.find( searchString )
	while ( findResult != null  && !(loopedOnce && !replaceAll))
	{
		string part1 = baseString.slice( 0, findResult )
		string part2 = baseString.slice( findResult + searchString.len(), baseString.len() )

		baseString = part1 + replaceString + part2
...

vs

...
string function GetMapDisplayDesc( string mapname )
{
	return "#" + mapname + "_CLASSIC_DESC"
}

///-----------------------------------------------------------------------------
/// Sends a string message to player
/// * `baseString` - The input string to search through
/// * `searchString` - Find this substring...
/// * `replaceString` - ...and replace with this substring
/// * `replaceAll` - Whether to replace all occurences or just the first
/// * `caseInsensitive` - Whether to consider casing (upper/lower)
///
/// Returns the updated string
///-----------------------------------------------------------------------------
string function StringReplace( string baseString, string searchString, string replaceString, bool replaceAll = false, bool caseInsensitive = false )
{
	bool loopedOnce = false
	string source = caseInsensitive ? baseString.tolower() : baseString
	var findResult = source.find( searchString )
	while ( findResult != null  && !(loopedOnce && !replaceAll))
	{
		string part1 = baseString.slice( 0, findResult )
		string part2 = baseString.slice( findResult + searchString.len(), baseString.len() )

		baseString = part1 + replaceString + part2
...

The Discord vote a while back was also slightly in favour of a format that doesn't have the decorators though our conclusion back when the vote was done was to combine the two most voted versions

@ASpoonPlaysGames
Copy link
Contributor

Ok so, yeah drop the decorators.

But also, do we actually need doc comments for squirrel functions? Squirrel is scripting, mostly not stuff that is reused

@GeckoEidechse
Copy link
Member Author

Ok so, yeah drop the decorators.

Done with c3eba08

But also, do we actually need doc comments for squirrel functions? Squirrel is scripting, mostly not stuff that is reused

The reason is to both improve readability and to then be able to auto-generate documentation directly from code ^^

@F1F7Y
Copy link
Member

F1F7Y commented Oct 30, 2023

So looking at this in the diff rn, I think we should drop the ///--------- decorators before/after doc comment cause it just adds more (unnecessary lines).

The entire point of the separators is to add more lines to make it visually easier to separate the comment from code, not all of us have syntax highlighting... I can see how it could get annoying to have to copy paste these and they dont add anything to the docs so whatever the hivemind wants we go with

@GeckoEidechse
Copy link
Member Author

The entire point of the separators is to add more lines to make it visually easier to separate the comment from code, not all of us have syntax highlighting...

Regarding syntax highlighting, we might wanna do a poll on dev setups people use to see what we should target. I'm pretty sure vscode is the highest (and that one has an extension for squirrel). Curious what is second. Probably Notepad++ but not sure if that has any highlighting for Squirrel.

(sidenote: the occasional lack of Syntax highlighting is also why I have a disliking for /* ... */ style comments :P)

I can see how it could get annoying to have to copy paste these and they dont add anything to the docs so whatever the hivemind wants we go with

For me it's mostly the visual clutter they add. I think the easiest solution is to just do a poll on what people prefer and go with that. Imma set one up.
In the end regardless of which one we go for, if we ever wanna change adding/removing the decorators should be quite easy.

@GeckoEidechse
Copy link
Member Author

Made a poll in #759

@GeckoEidechse
Copy link
Member Author

Made a poll in #759

Poll is currently at 7:2 in favour of no decorators so I'd say we do no decorators for now ^^

@GeckoEidechse GeckoEidechse removed the feedback wanted Feedback is wanted whether the changes by this PR are desired label Nov 5, 2023
@ASpoonPlaysGames ASpoonPlaysGames added the READY TO MERGE This mergeable right now label Dec 4, 2023
Copy link
Contributor

@ASpoonPlaysGames ASpoonPlaysGames left a comment

Choose a reason for hiding this comment

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

Code looks good (lol its a comment)

Since it's just a comment, testing isn't needed

@GeckoEidechse GeckoEidechse merged commit 0c5c9b1 into main Dec 4, 2023
6 checks passed
@GeckoEidechse GeckoEidechse deleted the initial-doc-comment-test branch December 4, 2023 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
READY TO MERGE This mergeable right now
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants