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

Fields without a tag can still change #7

Open
ansel1 opened this issue Feb 3, 2018 · 6 comments
Open

Fields without a tag can still change #7

ansel1 opened this issue Feb 3, 2018 · 6 comments

Comments

@ansel1
Copy link

ansel1 commented Feb 3, 2018

Example:

type Widget struct {
    B []bytes
}

defaults.SetDefaults(&Widget{})

The B field starts out nil, but will be set to an empty byte slice, even though that field has no "default" tag.

Looking at the code, I'm thinking either setDefaultValues should check for the presence of the tag, and skip the field if not present, or setDefaultValue should check the presence of the tag (after the zero value check).

Alternatively, if this was the intended behavior, then perhaps add a default:"-" option to explicitly skip the field? Or a flag in the Filler struct to toggle the behavior? I'd be happy to do a PR.

@mcuadros
Copy link
Owner

Sorry, this issue was buried in on a ton of others, the library should only affect fields with the tag. So yes feel free to do a PR.

@ansel1
Copy link
Author

ansel1 commented Apr 10, 2018

Do you still want to maintain compat with <go1.7? Cleanest impl uses a method added then.

@mcuadros
Copy link
Owner

It's ok to upgrade to 1.8 or 1.9

@ansel1
Copy link
Author

ansel1 commented Apr 16, 2018

There are several tests which expected the old behavior. There are a few categories:

  1. Several tests use example structs with no tags, but expect them to get filled anyway. I'm treating this as incorrect test criteria, and adding a tag to the sample struct to fix.
  2. Several tests construct the Filler{} struct without setting the Tag field. With the old behavior, a Filler with no Tag would still end up calling filler functions. It would be up to the implementation of the FillerFunc to fill the value, regardless of whether the tag was present or not. The tests are expecting those filler functions to be called, even if the struct is untagged or Filler.Tag is empty. I'm assuming the correct behavior if the field is not tagged is that the filler func should not be called. But in the case where the Filler.Tag field is empty, I'm wondering what you would prefer. It seems like an error to have an empty Tag field, so I can either assume that if Filler.Tag == "", I should use the default tag of "default". Other options: 1) have Filler.Fill() panic, 2) have Filler.Fill() return silently (effectively a no-op).
  3. The test for the "Factory()" method now fails. The "Factory Filler" is supposed to look for the "factory" tag, but the test uses the ExampleBasic struct, which uses the "default" tag. The test has been passing up until now, because the filler functions in the factory filler all ignore the tag entirely. They were always called for any field with a zero value, regardless of the tags on the field, and the filler functions just filled in random values, paying no attention to the tag. I can fix this test by changing the tag name, or using a different sample struct that has "factory" tags, but this feels like a more breaking change.

I could maintain more of the current behaviors. I could make it so that if Filler.Tag == "", then filler functions all get called regardless of tags, effectively reverting to the old behavior. And I would change the factory filler to make Tag == "". That would make most of the tests pass again without altering their assumptions, and would make the factory filler backward compatible.

@matheus-meneses
Copy link

matheus-meneses commented Nov 4, 2021

Hi guys,

It's still an issue :(

Do you have a workaround for this?

@ansel1
Copy link
Author

ansel1 commented Nov 4, 2021

Sorry, we ending up using a different library for this (creasty/defaults)

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

No branches or pull requests

3 participants