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

refactor config #90

Merged
merged 3 commits into from
Mar 27, 2023
Merged

refactor config #90

merged 3 commits into from
Mar 27, 2023

Conversation

duytiennguyen-okta
Copy link
Contributor

@duytiennguyen-okta duytiennguyen-okta commented Mar 20, 2023

Best practices code, switching config over to query object/method style of behavior. Let the config tell us its state, don't make decisions about state outside of the config.

@monde
Copy link
Collaborator

monde commented Mar 21, 2023

@duytiennguyen-okta can you fill out a description of the PR with the background on the changes?

I'm not sure if I agree with having a bunch more code for getters to do information hiding on the config struct. The config struct is for the application's benefit, we own this code, we aren't exposing it as a public SDK. Outside callers aren't going to be modifying values on the public fields. Even our use of a linter for code quality is forcing listing extra comments for self evident values, for example // OrgDomain --. Also, my understanding of a good time to have getters are to define them on an interface as a contract for the domain logic the interface represents. I'm on the fence on the getters.

That said, I do see the value in having shelf checking setters for input validation. But if you are going to the extra effort to improve input validation why don't you also have unit tests on your setters?

err = cfg.SetAWSSessionDuration(attrs.AWSSessionDuration)
	if err != nil {
		return nil, err
	}
``

Copy link
Collaborator

@monde monde left a comment

Choose a reason for hiding this comment

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

:shipit:

@duytiennguyen-okta duytiennguyen-okta merged commit af59e8d into master Mar 27, 2023
@duytiennguyen-okta duytiennguyen-okta deleted the refactor branch March 27, 2023 17:21
@monde monde mentioned this pull request May 2, 2023
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