Skip to content

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

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.

Sign up for GitHub

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

Merged
mwbrooks merged 2 commits into from
Jun 20, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
31fed26
refactor(config): destructify config.GetProjectDirPath(fs, os)
mwbrooks Jun 14, 2025
be4d1c9
Merge branch 'main' into mwbrooks-destructify-config-example
mwbrooks Jun 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions internal/config/project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ type ProjectConfigManager interface {
ReadProjectConfigFile(ctx context.Context) (ProjectConfig, error)
WriteProjectConfigFile(ctx context.Context, projectConfig ProjectConfig) (string, error)
ProjectConfigJSONFileExists(projectDirPath string) bool
GetProjectDirPath() (string, error)
Copy link
Member Author

@mwbrooks mwbrooks Jun 16, 2025

Choose a reason for hiding this comment

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

note: Removing GetProjectDirPath from the interface.


Cache() cache.Cacher
}
Expand Down Expand Up @@ -95,7 +94,7 @@ func (c *ProjectConfig) InitProjectID(ctx context.Context, overwriteExistingProj
defer span.Finish()

// Check that current directory is a project
if _, err := c.GetProjectDirPath(); err != nil {
if _, err := GetProjectDirPath(c.fs, c.os); err != nil {
return "", err
}

Expand Down Expand Up @@ -245,7 +244,7 @@ func (c *ProjectConfig) ReadProjectConfigFile(ctx context.Context) (ProjectConfi

var projectConfig ProjectConfig

projectDirPath, err := c.GetProjectDirPath()
projectDirPath, err := GetProjectDirPath(c.fs, c.os)
if err != nil {
return projectConfig, err
}
Expand Down Expand Up @@ -285,7 +284,7 @@ func (c *ProjectConfig) WriteProjectConfigFile(ctx context.Context, projectConfi
return "", err
}

projectDirPath, err := c.GetProjectDirPath()
projectDirPath, err := GetProjectDirPath(c.fs, c.os)
if err != nil {
return "", err
}
Expand All @@ -308,21 +307,21 @@ func (c *ProjectConfig) ProjectConfigJSONFileExists(projectDirPath string) bool

// 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) {
Copy link
Member Author

@mwbrooks mwbrooks Jun 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zimeg zimeg Jun 18, 2025

Choose a reason for hiding this comment

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

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 🙏 ✨

Copy link
Member Author

@mwbrooks mwbrooks Jun 20, 2025

Choose a reason for hiding this comment

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

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!

currentDir, _ := c.os.Getwd()
func GetProjectDirPath(fs afero.Fs, os types.Os) (string, error) {
currentDir, _ := os.Getwd()

// Verify project-level hooks.json exists
projectHooksJSONPath := GetProjectHooksJSONFilePath(currentDir)
if _, err := c.fs.Stat(projectHooksJSONPath); os.IsNotExist(err) {
if _, err := fs.Stat(projectHooksJSONPath); os.IsNotExist(err) {

// Fallback check for slack.json and .slack/slack.json file
// DEPRECATED(semver:major): remove both fallbacks next major release
projectSlackJSONPath := filepath.Join(currentDir, "slack.json")
if _, err := c.fs.Stat(projectSlackJSONPath); err == nil {
if _, err := fs.Stat(projectSlackJSONPath); err == nil {
return currentDir, nil
}
projectDotSlackSlackJSONPath := filepath.Join(currentDir, ".slack", "slack.json")
if _, err := c.fs.Stat(projectDotSlackSlackJSONPath); err == nil {
if _, err := fs.Stat(projectDotSlackSlackJSONPath); err == nil {
return currentDir, nil
}
return "", slackerror.New(slackerror.ErrInvalidAppDirectory)
Expand All @@ -333,7 +332,7 @@ func (c *ProjectConfig) GetProjectDirPath() (string, error) {

// Cache loads the cached project values
func (c *ProjectConfig) Cache() cache.Cacher {
path, err := c.GetProjectDirPath()
path, err := GetProjectDirPath(c.fs, c.os)
if err != nil {
return &cache.Cache{}
}
Expand Down
5 changes: 0 additions & 5 deletions internal/config/project_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,6 @@ func (m *ProjectConfigMock) ProjectConfigJSONFileExists(projectDirPath string) b
return args.Bool(0)
}

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

Comment on lines -86 to -90
Copy link
Member Author

@mwbrooks mwbrooks Jun 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zimeg zimeg Jun 18, 2025

Choose a reason for hiding this comment

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

@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! 🧪

Copy link
Member Author

@mwbrooks mwbrooks Jun 20, 2025

Choose a reason for hiding this comment

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

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.

// Cache returns a persistent mock cache
func (m *ProjectConfigMock) Cache() cache.Cacher {
args := m.Called()
Expand Down
10 changes: 4 additions & 6 deletions internal/config/project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ func Test_ProjectConfig_ReadProjectConfigFile(t *testing.T) {
addProjectMocks(t, fs)
projectConfig := NewProjectConfig(fs, os)

projectDirPath, err := projectConfig.GetProjectDirPath()
projectDirPath, err := GetProjectDirPath(fs, os)
require.NoError(t, err)
projectConfigFilePath := GetProjectConfigJSONFilePath(projectDirPath)

Expand Down Expand Up @@ -462,8 +462,7 @@ func Test_ProjectConfig_GetProjectDirPath(t *testing.T) {
err := fs.Remove(GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory))
require.NoError(t, err)

projectConfig := NewProjectConfig(fs, os)
projectDirPath, err := projectConfig.GetProjectDirPath()
projectDirPath, err := GetProjectDirPath(fs, os)
require.Error(t, err)
require.Empty(t, projectDirPath)
})
Expand All @@ -478,8 +477,7 @@ func Test_ProjectConfig_GetProjectDirPath(t *testing.T) {
// Use project mocks to create project in filesystem
addProjectMocks(t, fs)

projectConfig := NewProjectConfig(fs, os)
projectDirPath, err := projectConfig.GetProjectDirPath()
projectDirPath, err := GetProjectDirPath(fs, os)
require.NoError(t, err)
require.Equal(t, slackdeps.MockWorkingDirectory, projectDirPath) // MockWorkingDirectory is the test's project directory
})
Expand Down Expand Up @@ -796,6 +794,6 @@ func addProjectMocks(t require.TestingT, fs afero.Fs) {
require.NoError(t, err)

// Fixture: project/.slack/hooks.json file
err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0600)
err = afero.WriteFile(fs, GetProjectHooksJSONFilePath(slackdeps.MockWorkingDirectory), []byte("{}\n"), 0644)
Comment on lines -799 to +797
Copy link
Member

@zimeg zimeg Jun 18, 2025

Choose a reason for hiding this comment

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

👁️‍🗨️ ✨

require.NoError(t, err)
}
3 changes: 2 additions & 1 deletion internal/slackdeps/os_mock.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
const MockHomeDirectory = "/Users/user.name"

// MockWorkingDirectory is the default returned by Getwd().
const MockWorkingDirectory = "/Users/user.name/app"
const MockWorkingDirectory = "/Users/user.name/project"
Copy link
Member Author

@mwbrooks mwbrooks Jun 16, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@zimeg zimeg Jun 18, 2025

Choose a reason for hiding this comment

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

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


// MockCustomConfigDirectory is a custom config directory for ConfigDirFlag
const MockCustomConfigDirectory = "/tmp/tmp.user.name.123"
Expand All @@ -50,6 +50,7 @@ func (m *OsMock) AddDefaultMocks() {
m.On("UserHomeDir").Return(MockHomeDirectory, nil)
m.On("GetExecutionDir").Return(MockHomeDirectory, nil)
m.On("SetExecutionDir", mock.Anything)
m.On("IsNotExist", nil).Return(false)
Copy link
Member Author

@mwbrooks mwbrooks Jun 16, 2025

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

@zimeg zimeg Jun 18, 2025

Choose a reason for hiding this comment

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

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! 🚢

Copy link
Member Author

@mwbrooks mwbrooks Jun 20, 2025

Choose a reason for hiding this comment

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

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.

m.On("IsNotExist", mock.Anything).Return(true)
m.On("Exit", mock.Anything).Return()
m.On("Stat", mock.Anything).Return()
Expand Down
Loading