refactor(config): destructify config.GetProjectDirPath(fs, os) #130

mwbrooks · 2025-06-16T19:22:28Z

Summary

Proposal to refactor the package config from structs to standalone functions.

This pull request refactors the function GetProjectDirPath in package config from a struct to a standalone function. This is a small example to showcase the changes involve in the factor and explain the benefits. If we like this direction, then I'll follow-up with large pull requests that make the changes for all of internal/config/project.go.

Before:

err := clients.Config.ProjectConfig.GetProjectDirPath()

After:

err := config.GetProjectDirPath(fs, os)

Benefits:

More idiomatic Golang
Separate data (fs) from behaviour (the function)
Makes dependencies explicit in the function signature
Follows Golang's preference for small, focused functions over methods on large structs
More accurate tests that use a memory-based file system that represents the production environments
Lighter clients struct overall

Drawbacks:

Must update tests to mock the memory-based file system (fs.MemMapFs) instead of the methods (projectConfigMock.On("GetManifestSource", mock.Anything,)...)
Challenging to untangle some of the package config methods, such as Cache() and SetSurveyConfig

Requirements

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2025-06-16T19:24:38Z

Codecov Report

Attention: Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.

Project coverage is 63.48%. Comparing base (554ff7f) to head (be4d1c9).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/config/project.go	77.77%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #130      +/-   ##
==========================================
+ Coverage   63.46%   63.48%   +0.01%     
==========================================
  Files         212      212              
  Lines       22345    22345              
==========================================
+ Hits        14182    14185       +3     
+ Misses       7080     7078       -2     
+ Partials     1083     1082       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

mwbrooks · 2025-06-16T19:22:58Z

internal/config/project.go

 	ReadProjectConfigFile(ctx context.Context) (ProjectConfig, error)
 	WriteProjectConfigFile(ctx context.Context, projectConfig ProjectConfig) (string, error)
 	ProjectConfigJSONFileExists(projectDirPath string) bool
-	GetProjectDirPath() (string, error)


note: Removing GetProjectDirPath from the interface.

mwbrooks · 2025-06-16T19:23:56Z

internal/config/project.go


 // GetProjectDirPath returns the path to the project directory or an error if not a Slack project
 // TODO(@mbrooks) Standardize the definition of a validate project directory and merge with `cmdutil.ValidProjectDirectoryOrExit`
-func (c *ProjectConfig) GetProjectDirPath() (string, error) {


note: Changing GetProjectDirPath from a method on the struct to a standalone function. Now the dependencies are injected as arguments instead of accessed as private struct fields.

Huge unlock in making it clear what's needed for these functions to complete! I like that "standard" dependencies are no longer kept within a struct here.

Organizing related functions into packages still seems ideal for separating most concerns. Perhaps a returned configuration object has attached methods? But I'm so open to going back and forth until a good balance is found 🙏 ✨

Good to know that this is resonating with you as well @zimeg! There's definitely some ergonomics that we'll need to figure out, whether returning a struct or better naming conventions. But at least the general direction sounds positive!

mwbrooks · 2025-06-16T19:25:22Z

internal/config/project_mock.go

-func (m *ProjectConfigMock) GetProjectDirPath() (string, error) {
-	args := m.Called()
-	return args.String(0), args.Error(1)
-}
-


note: Removing the mock. We don't use this mock, but for some future methods, it will require us set the correct data in the memory-based file system and then read it back to confirm it.

@mwbrooks This is an excellent change from testing "mocks" to "fixtures" where we can exercise all of the logic with a certain set of known files! 🧪

Oooo, I like how you phrased that. From testing "mocks" to testing "fixtures." The fixtures feel more real, even though they require more setup and the unit tests run deeper down the stack.

mwbrooks · 2025-06-16T19:26:48Z

internal/slackdeps/os_mock.go


 // MockWorkingDirectory is the default returned by Getwd().
-const MockWorkingDirectory = "/Users/user.name/app"
+const MockWorkingDirectory = "/Users/user.name/project"


note: I changed this to /project because it was a little weird to read in test output. We're creating a new project, not a Slack app. So it makes sense to have the directory called project.

🗣️ Past readings have made me a believer in mock data matching what might be expected in production! Nice change once more.

mwbrooks · 2025-06-16T19:27:52Z

internal/slackdeps/os_mock.go

 	m.On("UserHomeDir").Return(MockHomeDirectory, nil)
 	m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
 	m.On("SetExecutionDir", mock.Anything)
+	m.On("IsNotExist", nil).Return(false)


note: In order for tests to pass, we need our mock for os.IsNotExist(err) to return false when err = nil. In most cases, we don't need a mock here and could use the real IsNotExist 🤔

In most cases, we don't need a mock here and could use the real IsNotExist

Heh I had this exact same though! Now that we're moving towards fixtures that might be a more stable change to avoid mocking strangeness from odd defaults. No blocker for these changes though! 🚢

Great point that as we use more fixtures instead of mocks, we may not need to mock a IsNotExist scenario. Instead, we can setup the fixtures to satisfy that edge-case.

zimeg

@mwbrooks Thanks so much for prototyping these refactors 🎁

At a glance this change makes a lot of sense and I'm glad to find fewer hidden dependencies of this function.

I'm thinking about which methods we do want to keep in a structure, perhaps those with stateful data, but also am open to discussing this here or in a later change 👾

Feel free to merge when the time is right! I'm open to reviewing similar changes of course!

zimeg · 2025-06-18T16:50:32Z

internal/config/project.go


 // GetProjectDirPath returns the path to the project directory or an error if not a Slack project
 // TODO(@mbrooks) Standardize the definition of a validate project directory and merge with `cmdutil.ValidProjectDirectoryOrExit`
-func (c *ProjectConfig) GetProjectDirPath() (string, error) {


Huge unlock in making it clear what's needed for these functions to complete! I like that "standard" dependencies are no longer kept within a struct here.

Organizing related functions into packages still seems ideal for separating most concerns. Perhaps a returned configuration object has attached methods? But I'm so open to going back and forth until a good balance is found 🙏 ✨

zimeg · 2025-06-18T21:23:52Z

internal/config/project_mock.go

-func (m *ProjectConfigMock) GetProjectDirPath() (string, error) {
-	args := m.Called()
-	return args.String(0), args.Error(1)
-}
-


@mwbrooks This is an excellent change from testing "mocks" to "fixtures" where we can exercise all of the logic with a certain set of known files! 🧪

zimeg · 2025-06-18T21:24:23Z

internal/config/project_test.go

-	err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600)
+	err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0644)


👁️‍🗨️ ✨

zimeg · 2025-06-18T21:25:10Z

internal/slackdeps/os_mock.go


 // MockWorkingDirectory is the default returned by Getwd().
-const MockWorkingDirectory = "/Users/user.name/app"
+const MockWorkingDirectory = "/Users/user.name/project"


🗣️ Past readings have made me a believer in mock data matching what might be expected in production! Nice change once more.

zimeg · 2025-06-18T21:26:22Z

internal/slackdeps/os_mock.go

 	m.On("UserHomeDir").Return(MockHomeDirectory, nil)
 	m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
 	m.On("SetExecutionDir", mock.Anything)
+	m.On("IsNotExist", nil).Return(false)


In most cases, we don't need a mock here and could use the real IsNotExist

Heh I had this exact same though! Now that we're moving towards fixtures that might be a more stable change to avoid mocking strangeness from odd defaults. No blocker for these changes though! 🚢

mwbrooks · 2025-06-20T23:02:31Z

Thanks for the review @zimeg! It's good to hear that this is a positive step forward in your mind. So, I'll go ahead and merge it then follow-up with a wider refactor for other functions.

I'm thinking about which methods we do want to keep in a structure, perhaps those with stateful data, but also am open to discussing this here or in a later change 👾

We'll be forced to think about this soon, I think. There are a few functions that aren't as straight forward, such as config.ProjectConfig.Cache() that returns a cache struct. I imagine these will start to nudge out an answer and I agree, the stateful data will probably be the key. 🔑

refactor(config): destructify config.GetProjectDirPath(fs, os)

31fed26

mwbrooks added this to the Next Release milestone Jun 16, 2025

mwbrooks self-assigned this Jun 16, 2025

mwbrooks requested a review from a team as a code owner June 16, 2025 19:22

mwbrooks added code health M-T: Test improvements and anything that improves code health semver:patch Use on pull requests to describe the release version increment labels Jun 16, 2025

mwbrooks commented Jun 16, 2025

View reviewed changes

zimeg approved these changes Jun 18, 2025

View reviewed changes

Merge branch 'main' into mwbrooks-destructify-config-example

be4d1c9

mwbrooks merged commit 4d342d7 into main Jun 20, 2025
6 checks passed

mwbrooks deleted the mwbrooks-destructify-config-example branch June 20, 2025 23:08

mwbrooks mentioned this pull request Jun 23, 2025

refactor(config): destructify project-level getter and setters in package config #138

Merged

2 tasks

		err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600)
		err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0644)

refactor(config): destructify config.GetProjectDirPath(fs, os) #130

refactor(config): destructify config.GetProjectDirPath(fs, os) #130

Uh oh!

Conversation

mwbrooks commented Jun 16, 2025

Summary

Requirements

Uh oh!

codecov bot commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zimeg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mwbrooks commented Jun 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jun 16, 2025 •

edited

Loading