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

chore: go 1.23 upgrade, README fix, docstring additions, & go styling updates #28

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

sheikhrachel
Copy link
Contributor

@sheikhrachel sheikhrachel commented Oct 17, 2024

Purpose

I've contributed to a couple of Netflix OSS projects in the past (weep and consoleme), and wanted to give some TLC to this package to bring it up-to-date with fixes to the README examples, an update to the Go version, and code-style improvements to align with updates to the Go language in recent updates.

Happy to discuss / elaborate on any of my proposals in this change / sidecar scope into separate changes if desired 🙏

Updating the project's Go version

Readme fixes

  • Fixes the currently broken README examples
    • env import aliases is redundant, as the import is targeted at the name after the hyphen old source on this behaviour
    • Adds an explanation for the required linting that fails when calling the usage example as-is to surface attention around the built-in validation logic
    • Adds the missing package main block to the second example to allow for simpler copy/pasting into a playground or project
    • Adopts the more modern conditional var-scoping to the examples
    • Ensures the calls to Marshal and UnmarshalFromEnviron take pointers to the Environment/Config structs to prevent the ErrInvalidValue error from being returned

Test semantics

  • Takes advantage of t.Parallel() to allow unit tests across the project to be run in parallel

Conditional var-scoping

  • Takes advantage of flow controls for instances where function call returns are used immediately to simplify how variables are scoped throughout functions and tests

Increased usage of continue

  • Updates a number of if-else blocks to simplify them down to an if-contine-block with a subsequent default case

Range over int

  • Takes advantage of the new Go language improvements that deprecate the need to write out for loops with for i := 0; i < 5; i++ by simply declaring the integer range to iterate over, ex. for i := range 5

Consts & Docstrings

  • Improves docstring coverage in env.go to provide more clarity into the library / improve switch performance when parsing tags

@sheikhrachel
Copy link
Contributor Author

cc @patricksanders (👋) if you know of the right person to tag here for a review

@sheikhrachel sheikhrachel changed the title [chore] go 1.23.2 upgrade, README fix, docstring additions, & go styling updates chore: go 1.23.2 upgrade, README fix, docstring additions, & go styling updates Oct 18, 2024
@patricksanders
Copy link
Contributor

Hey @sheikhrachel, nice to see you around these parts. Thanks for the PR! I'll take a look.

go.mod Outdated
@@ -1,3 +1,3 @@
module github.com/Netflix/go-env

go 1.22
go 1.23.2
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the .2 minor revision a hard requirement here? If not, let's just make this 1.23 to avoid things like #26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not necessary - updated to just go 1.23 and updated title/description to match ✅

@sheikhrachel sheikhrachel changed the title chore: go 1.23.2 upgrade, README fix, docstring additions, & go styling updates chore: go 1.23 upgrade, README fix, docstring additions, & go styling updates Oct 25, 2024
Copy link
Contributor

@patricksanders patricksanders left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the housekeeping!

@patricksanders patricksanders merged commit ec74a1d into Netflix:master Oct 25, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants