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

[mwbrooks]

Summary

This pull request continues PR #130 by de-struct-ifying the following project-level methods in package config:

• config.ProjectConfigJSONFileExists(...)
• config.WriteProjectConfigFile(...)
• config.ReadProjectConfigFile(...)
• config.SetManifestSource(...)

I've chosen these methods because their changes are relatively straight-forward and repetitive. The remaining methods require wider changes, so I'd like to isolate them to smaller, future PRs.

Changes

A common refactor looks like this:

Remove method from the interface:

- GetProjectDirPath() (string, error)

Update method to be a standalone function with dependencies as arguments:

- func (c *ProjectConfig) GetProjectDirPath() (string, error) {
+ func GetProjectDirPath(fs afero.Fs, os types.Os) (string, error) {

Update references to use the standalone function:

- projectDirPath, err := c.GetProjectDirPath()
+ projectDirPath, err := GetProjectDirPath(c.fs, c.os)

Remove method from the project mocks:

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

Update tests to use filesystem mock instead of project-config mock:

- projectConfigMock := &config.ProjectConfigMock{}
- projectConfigMock.On("GetProjectDirPath").Return(slackdeps.MockWorkingDirectory)
+ slackmock.CreateProject(t, ctx, clientsMock.Fs, clientsMock.Os, slackdeps.MockWorkingDirectory)

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]

Codecov Report

Attention: Patch coverage is 83.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 63.58%. Comparing base (0faf469) to head (e442e0f).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/config/project.go	89.47%	2 Missing ⚠️
cmd/app/link.go	0.00%	0 Missing and 1 partial ⚠️
internal/pkg/create/create.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
- Coverage   63.59%   63.58%   -0.02%     
==========================================
  Files         212      212              
  Lines       22348    22348              
==========================================
- Hits        14213    14209       -4     
- Misses       7054     7058       +4     
  Partials     1081     1081              

☔ 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.

[zimeg]

@mwbrooks LGTM! I was thinking more about where config will end up after similar changes and it's seeming good.

One comment I left was around perhaps unclear logic when calling a function, but these changes are shifting how I think about invocations:

The fs and os used have the context of the path started in!

This might not be surprising to you, but using arguments instead of internal structures surfaces this more for me 😉

[zimeg]

👁️‍🗨️ This might be because I'm used to the before version of this code, but to me this:

projectConfig, err := clients.Config.ProjectConfig.ReadProjectConfigFile(ctx)

Hid the magic of how the project path was found. I realize now we're doing the same thing with arguments shared instead, but it wasn't obvious how fs and os were used to read the configuration to me...

projectConfig, err := config.ReadProjectConfigFile(ctx, clients.Fs, clients.Os)

I do think this is good improvement for testing and using this package overall and it's not so confusing IRL but I wanted to share an initial thought here!

[mwbrooks]

I agree @zimeg, this function has always felt magical to me as well.

Behind the scenes, it's using the current working directory as the project path, but that's bad practice imo. This kind of logic makes it difficult for us to support running commands in sub-directories and it confusing to read as a maintainer.

We should definitely improve it in the future!

[zimeg]

🔍 I'm super curious if adding options of common fixtures for this might be tried later, or if that's something that'll be left to test setups?

The tests above I think are clear, and as more of clients is destructured this might read so well:

slackmock.CreateProject(t, ctx, clients.Fs, clients.Os, slackdeps.MockWorkingDirectory)

_, err := config.WriteProjectConfigFile(ctx, clients.Fs, clients.Os, tt.projectConfig)
require.NoError(t, err)

[mwbrooks]

🧠 Interesting! What do you have in mind? Are you thinking project fixtures like "project-deno", "project-bolt-js", "project-missing-hooks", "project-missing-dotslack", etc?

[zimeg]

@mwbrooks Those are all seeming solid to me and appear most in tests IIRC - I do wonder if adding these now or much later makes the most sense in case other patterns appear, but those cases are still so good to have 😌 🎉

[mwbrooks]

@zimeg Sorry, I meant to respond before I hit merge 🤦🏻 I think we can add these later, but perhaps we should try to experiment with it while we're doing the destructify work, in case it's not as elegant as we hope?

[mwbrooks]

Thanks so much for the PR review @zimeg! Keep the honest thoughts coming with this destructify work. The goal is to make the code easier to use and maintain, which reducing things like circular dependencies and passing "god objects." If you ever get a sense that this isn't the right direction - particularly from the testing perspective - then we can stop and re-eval!