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

use source readers #76

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

use source readers #76

wants to merge 2 commits into from

Conversation

pablochacin
Copy link
Contributor

@pablochacin pablochacin commented Mar 12, 2025

Use the source's reader when analyzing dependencies.

The source's reader comes from the content or from reading the source's file (this applies to archive, script, and manifest). For the Environment source, it is assumed that the content is always present.

Breaking changes:

  • If the script source is given a name but not content, the name is used as the script's file and its content is tried to be read. Before this was not the case because it was assumed the content was already read always.
  • The signature of the FindManifest function changed and it now does not return the content, only the path.

Signed-off-by: Pablo Chacin <[email protected]>
@pablochacin pablochacin marked this pull request as ready for review March 12, 2025 16:22
@pablochacin pablochacin requested a review from a team as a code owner March 12, 2025 16:22
@pablochacin pablochacin requested review from szkiba and removed request for a team March 12, 2025 16:22
@@ -67,7 +67,7 @@ func Test_scriptAnalyzer(t *testing.T) {
fn := scriptAnalyzer(src)
deps, err := fn()

require.NoError(t, err)
require.Error(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't that right, that now we expect error? 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I documented in the PR as a breaking change

If the script source is given a name but not content, the name is used as the script's file and its content is tried to be read. Before this was not the case because it was assumed the content was already read always

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, yes makes totally sense!

Copy link
Contributor

@olegbespalov olegbespalov left a comment

Choose a reason for hiding this comment

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

Overall looks good 🚀 One question left in-line

@@ -163,27 +144,38 @@ func loadEnv(opts *Options) {
opts.Env.Contents = []byte(value)
}

func findManifest(filename string) ([]byte, string, bool, error) {
func (opts *Options) findManifest() error {
path, found, err := findManifest(opts.Script.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that old code has it too, but why do we search manifest, by using opts.Script.Name ? 🤔

I think it could confuse a bit of future contributors and if that's, that's the right way, it's good to have a comment here

options.go Outdated
@@ -163,27 +144,38 @@ func loadEnv(opts *Options) {
opts.Env.Contents = []byte(value)
}

func findManifest(filename string) ([]byte, string, bool, error) {
func (opts *Options) findManifest() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that it's worth giving the name that represents that state is changes, like initializeManifest or configureManifest

Signed-off-by: Pablo Chacin <[email protected]>
@olegbespalov
Copy link
Contributor

LGTM! 🚀

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