Skip to content

Fixes blank line warnings and the easy obsolete warnings #2384

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Fixes blank line warnings and the easy obsolete warnings #2384

wants to merge 3 commits into from

Conversation

@Laboredih123
Copy link
Contributor

@Laboredih123 Laboredih123 commented Sep 3, 2025

we have a LOT of warnings on the opendream folder, bout time we shave em down a bit.

@boring-cyborg boring-cyborg bot added Client Involves the OpenDream client Compiler Involves the OpenDream compiler Runtime Involves the OpenDream server/runtime labels Sep 3, 2025
@github-actions
Copy link

github-actions bot commented Sep 3, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-advanced-security[bot]
github-advanced-security bot found potential problems Sep 3, 2025
View reviewed changes
OpenDreamRuntime/Procs/DMOpcodeHandlers.cs
}

connection?.BrowseResource(file, filename.IsNull ? Path.GetFileName(file.ResourcePath) : filename.GetValueAsString());
connection?.BrowseResource(file, filename.IsNull ? Path.GetFileName(file.ResourcePath) : filename.MustGetValueAsString());

Check warning

Code scanning / InspectCode

Possible null reference argument for a parameter. Warning

Possible null reference argument for parameter 'filename' in 'OpenDreamRuntime.DreamConnection.BrowseResource'
@wixoaGit
Copy link
Member

wixoaGit commented Sep 5, 2025

Did you ensure the changes from GetValueAs() -> MustGetValueAs() are correct? The reason the new method was created was so we could go through each use of the deprecated version, and make sure throwing an exception on an invalid value type was the correct behavior.

@Laboredih123
Copy link
Contributor Author

Laboredih123 commented Sep 5, 2025 •
edited
Loading

getvalueasinteger and getvalueasstring just call their mustgetvalueas versions and nothing else

@wixoaGit
Copy link
Member

wixoaGit commented Sep 5, 2025

Yes, but they haven't already been removed because each use needs to be checked if BYOND errors on an invalid value, or coerces it to something else.

For example, statpanel() might treat the Panel argument as the default panel if it's not a string (I haven't tested this). Using MustGetValueAsString() here would error instead.

ZeWaka
ZeWaka reviewed Sep 5, 2025
View reviewed changes
Copy link
Collaborator

@ZeWaka ZeWaka left a comment

Choose a reason for hiding this comment

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

�

@ike709
Copy link
Collaborator

ike709 commented Sep 5, 2025 •
edited
Loading

To rephrase it, every usage of GetValueAs___() is essentially a TODO item to go test the BYOND behavior of passing not that type to whatever is using it. You can't replace one of those calls without extensively testing the behavior of every value type in BYOND.

@github-actions
Copy link

github-actions bot commented Nov 3, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@wixoaGit
Copy link
Member

wixoaGit commented Nov 25, 2025

Feel free to reopen this if you come back to it.

@wixoaGit wixoaGit closed this Nov 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@ZeWaka ZeWaka ZeWaka left review comments

@amylizzle amylizzle amylizzle approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Client Involves the OpenDream client Compiler Involves the OpenDream compiler Merge Conflict Runtime Involves the OpenDream server/runtime size/L

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Laboredih123 @wixoaGit @ike709 @amylizzle @ZeWaka