Skip to content

Conversation

harrisonhjones
Copy link
Contributor

This PR primarily addresses #3 by allowing for the timestamp to be customized when creating the logger. Instead of bumping the MV or creating a breaking change I introduced a new method, NewWith, which uses functional options to customize the logger. New and NewFor were updated to simply call NewWith with the correct values to keep their output consistent.

The PR also:

  1. Deprecates New and NewFor. I could see an argument to NOT deprecate New so let me know if you'd like that designation removed.
  2. Added a small comment in .gitignore to call out what .idea/ is. IMO this line shouldn't even be there and should instead be added to the developer's local .gitignore but not a hill I'm willing to die on :)
  3. Updated the readme and formatted it with prettier.
  4. Added tests for NewWith.
  5. Added / updated some doc comments so golint would stop complaining (as much).

@harrisonhjones
Copy link
Contributor Author

I'm not sure why the lint test is failing; based on the logs it looks like it passed?

@harrisonhjones
Copy link
Contributor Author

(clicked the wrong button)

@prozz
Copy link
Owner

prozz commented Feb 15, 2021

I'll try to fix linter today.

I'll also look into MR. From a quick glance I think we may just need single New that will accept optional functional options. It won't be a breaking change, but will also simplify the API.

@prozz
Copy link
Owner

prozz commented Feb 15, 2021

I used emf.NewFor exclusively for tests, so I don't think we should continue using it. As long as we are not 1.0 I'd just refactor old stuff out. Any recommendation on how to bump the version @harrisonhjones ?

@harrisonhjones
Copy link
Contributor Author

I personally don't have much an issue with removing NewFor and New and renaming NewWith to just New however it is technically a backwards incompatible change. Any removal of any exported is always backwards incompatible and, interesting, so is adding a variadic param to a function. Making these changes could break consumers.

If we don't want to do that we'd just need to follow something like this: https://blog.jetbrains.com/go/2020/03/25/working-with-go-modules-versioning/#major-version-upgrading or https://github.com/golang/go/wiki/Modules#releasing-modules-v2-or-higher. I would recommend the following (untested but seems reasonable):

  1. Rename the master branch to v1.
  2. Create a new v2 branch and put this change on it.
  3. Change go.mod's module github.com/prozz/aws-embedded-metrics-golang to module github.com/prozz/aws-embedded-metrics-golang/v2.
  4. Update tests to use the new import statement.
  5. Merge all commits into a single "Allow customization of the logger" commit
  6. Tag the commit with something like git tag v2.0
  7. Push to github.

@prozz
Copy link
Owner

prozz commented Feb 15, 2021

I know what you mean and I'd follow a protocol, but I still consider this lib to be a beta for some reason. I think introducing v1 and v2 that early may be more confusing than a tiny API change.

I hope I won't offend anyone by merging what's in front of us and just bumping version to 1.0 (it is a major version bump so may introduce breaking changes). Planned to bump to 1.0 after EC2 default metrics/dimensions will be sorted (see TODO in readme), but I'm not a heavy EC2 user, so cannot fix it properly.

@prozz prozz merged commit e4d4fd5 into prozz:master Feb 15, 2021
@harrisonhjones harrisonhjones deleted the feature-allow-customization-of-logger branch February 15, 2021 19:51
@harrisonhjones
Copy link
Contributor Author

Sounds good. Thanks for merging!

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