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

all: mention associated factory method for msgFieldInclusion #303

Merged
merged 3 commits into from Oct 18, 2018

Conversation

adamdecaf
Copy link
Member

This seems useful for people. If y'all think so too I'll do the whole project.

This should help users avoid errors by using the factory methods
rather than the plain structs.
@adamdecaf
Copy link
Member Author

All updated!

Copy link
Member

@wadearnold wadearnold left a comment

Choose a reason for hiding this comment

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

Generating error messages that may lead the user to solve what went wrong! Crazy! ;-)

This is really nice Adam!

@bkmoovio
Copy link
Contributor

bkmoovio commented Oct 18, 2018

Adam, Did you do this for all msgFieldInclusion already?

Wondering if adding %v (%s) to msgFieldInclusion and pass in the type so something like:

msgFieldInclusion      = "is a mandatory field and has a default value for %v"
    
    	if addenda02.TypeCode == "" {
	msg := fmt.Sprintf(msgFieldInclusion, "Addenda02")
	return &FieldError{FieldName: "TypeCode", Value: addenda02.TypeCode, Msg: msg}
}

Just banter... not critical

In other words this applies to all types that return msgfieldInclusion not just addenda types.

For example:

if ed.recordType == "" {
	return &FieldError{FieldName: "recordType", Value: ed.recordType, Msg: msgFieldInclusion}
}
if ed.TransactionCode == 0 {
	return &FieldError{FieldName: "TransactionCode", Value: strconv.Itoa(ed.TransactionCode), Msg: msgFieldInclusion}
}

@bkmoovio
Copy link
Contributor

bkmoovio commented Oct 18, 2018

It's in FileHeader also,

func (fh *FileHeader) fieldInclusion() error {
if fh.recordType == "" {
return &FieldError{FieldName: "recordType", Value: fh.recordType, Msg: msgFieldInclusion}
}
if fh.ImmediateDestination == "" {
return &FieldError{FieldName: "ImmediateDestination", Value: fh.ImmediateDestinationField(), Msg: msgFieldInclusion}
}

We may want to consider removing the "and has a default value", as it has evolved to returning an error for fields that are mandatory. Some still have a default value (recordType). I didn't think that this was critical path, that we could work on in another version, but with the API, it would bump up in priority.

@bkmoovio
Copy link
Contributor

bkmoovio commented Oct 18, 2018

I see you did do the others besides Addenda*. nice, so yeah its all good, We still may want to consider removing "and has a default value". For the alternative solution we would have to check e.FieldName

@adamdecaf
Copy link
Member Author

I thought about adding a %v to msgFieldInclusion, but didn't go down that route because I thought it was a bit too magical. We could do it though.

@bkmoovio
Copy link
Contributor

it's good as is. Thinking out loud...

@adamdecaf adamdecaf merged commit cecf4b9 into moov-io:master Oct 18, 2018
@adamdecaf adamdecaf deleted the errors-mention-builders branch October 18, 2018 19:33
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.

None yet

3 participants