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

deprecate unneeded utils.Testing #4350

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

abe-winter
Copy link
Member

@abe-winter abe-winter commented Sep 4, 2024

What changed

  • use testing.Testing() inside utils.Testing(), deprecate our wrapper

Why

This is in the standard library now

@viambot viambot added the safe to test This pull request is marked safe to test from a trusted zone label Sep 4, 2024
@abe-winter abe-winter marked this pull request as ready for review September 6, 2024 17:49
Comment on lines 48 to 52
// Testing returns true when you are running in test suite.
// Deprecated: this is in the standard library now.
func Testing() bool {
// TODO switch to official testing.Testing method when we are on go 1.21
return flag.Lookup("test.v") != nil
return testing.Testing()
}
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this is being used by external users. Seems like it is only used internally in utils/test_detector/main.go

Not sure how we go about deleting unused but public code. (we are pre v1 so maybe find to delete)

Copy link
Member Author

Choose a reason for hiding this comment

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

ideally do one release with it deprecated, next release removes? can be convinced to delete now if you think that's the move

Copy link
Member

Choose a reason for hiding this comment

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

I doubt people are upgrading every single week to the new version. Unless we want to deprecate this now and remove in 1-2 months (or never remove it), I doubt it makes a difference if we wait a week to remove.

Safest is just to deprecate. But I would hope that we could just remove.

Copy link
Member

Choose a reason for hiding this comment

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

Not too involved in rdk releases nowadays

Copy link
Member Author

Choose a reason for hiding this comment

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

will leave as-is and make it a problem for future me

@abe-winter abe-winter merged commit 3b5bc06 into viamrobotics:main Sep 6, 2024
19 checks passed
@abe-winter abe-winter deleted the official-testing branch September 6, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
safe to test This pull request is marked safe to test from a trusted zone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants