search

buildpacks / libcnb

A non-opinionated language binding for the Cloud Native Buildpack Buildpack and Extension specifications
Apache License 2.0
31 stars 13 forks source link

Removes the Application struct #102

Closed ForestEckhardt closed 2 years ago

ForestEckhardt commented 2 years ago

Signed-off-by: Forest Eckhardt feckhardt@pivotal.io

dmikusa commented 2 years ago

I'd be OK with removing the Application struct, but I'm not a fan of the WorkingDir name. I understand your logic, trying to make it match the terminology that's in the spec, but IMHO it makes the API more confusing to work with.

As a buildpack developer, right now, I can say context.Application.Path and get the path to the application that's being built. It's very clear. I'd be OK with shortening that. Having context.Application or context.ApplicationPath just be the path as a string. Especially since there's nothing else on the Application object. Keep it simple, makes sense.

Changing to context.WorkingDir seems like a step too far to me. I don't feel like that is clear in terms of what it points to. As a build path author, I want the application location for all kinds of reasons. I'm thinking in terms of applications. If I'm using intellisense or the godocs, something that says "application" is going to be a big clue. WorkingDir not as much. That could be the app, it could be some other temp directory.

My $0.02.

samj1912 commented 2 years ago

I agree with @dmikusa-pivotal on the naming choice - happy if the rename is ctx.ApplicationPath - I would prefer not to call it ctx.Application since that was already a struct in the existing api and might be confusing.

For some background re:spec - the spec currently sets the working directory for buildpacks to be the same as the application directly - but if that were to change in the future (let's say we provide the path the application via an env var instead) it will require renaming the ctx.WorkingDir variable to something related to the application source code directory.

ForestEckhardt commented 2 years ago

@samj1912 @dmikusa-pivotal I am cool if y'all want the bike shed purple! I will put in a change right away.