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

using any over interface{} #189

Merged
merged 12 commits into from
Feb 19, 2024
Merged

using any over interface{} #189

merged 12 commits into from
Feb 19, 2024

Conversation

dltacube
Copy link
Collaborator

@dltacube dltacube commented Feb 18, 2024

Changelist

  • convert interface{} to any ref

  • convert time.Millisecond to time.UnixMilli() ref

  • something something initialisms ref

  • refactor for reconnect ref initial implementation: adding retry timer #188

Gonna move these to another PR

  • some config options should be based on whether or not private apis are enabled ref
  • debug log clean up ref
  • 🔪 😵‍💫 ❤️ ref

@dltacube
Copy link
Collaborator Author

I just realized there are exceptions when it comes to initialism in cases where the variable or function's scope needlessly changes from the original authors intent.

Will fix that.

@dltacube dltacube marked this pull request as ready for review February 18, 2024 21:59
@dltacube
Copy link
Collaborator Author

I reverted this commit but do we want to make changes like this here that change the scope of the function? 2c1a437

@trek-boldly-go
Copy link
Collaborator

Closes #184
@cnuss I can't merge because the last commit is from me. Can you approve and merge this?

Copy link
Collaborator

@cnuss cnuss left a comment

Choose a reason for hiding this comment

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

i love PRs that clean things up, approved!

@trek-boldly-go
Copy link
Collaborator

@tulir I think GH is going to require an approval from you on this one. I think it is the edit to the tapbacks file that triggered it.

@@ -1227,12 +1227,12 @@ func (bb *blueBubbles) wsUrl() string {
q.Add("transport", "websocket")
u.RawQuery = q.Encode()

url := u.String()
URL := u.String()
Copy link
Member

Choose a reason for hiding this comment

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

Local variables (and in general anything that isn't exported from the package) should start with lowercase, so this was correct already before

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I noticed I was changing the scope on some variables with this commit (and undid that change) 0df68f5 but missed this one.

I'll refactor that so we're not unintentionally exporting anything.

Copy link
Member

Choose a reason for hiding this comment

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

Variables inside functions are never exported, so having them start with uppercase is always wrong

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see, even if they're an initial. Thanks for clarifying that!

Copy link
Collaborator Author

@dltacube dltacube Feb 19, 2024

Choose a reason for hiding this comment

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

This PR should address it: #194

@dltacube dltacube deleted the refactoring branch February 19, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants