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

add some ADRs with the start of a Roc API for RBT #17

Merged
merged 32 commits into from Jul 29, 2022
Merged

Conversation

BrianHicks
Copy link
Member

this is already a lot and I still don't feel like it answers all the questions I'd want resolved! I'd love feedback from anyone, especially on the questions marked by checkboxes in the document.

Copy link
Sponsor Contributor

@rtfeldman rtfeldman left a comment

Choose a reason for hiding this comment

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

Love it! I left a few suggestions - feel free to consider them and then merge away when you're happy with this!

docs/adrs/005-jobs.md Outdated Show resolved Hide resolved
docs/adrs/005-jobs.md Outdated Show resolved Hide resolved
docs/adrs/005-jobs.md Outdated Show resolved Hide resolved
docs/adrs/005-jobs.md Outdated Show resolved Hide resolved
docs/adrs/005-jobs.md Outdated Show resolved Hide resolved
@BrianHicks
Copy link
Member Author

hey @rtfeldman I've redone this after #18 merged. Quite a few changes here. Would you mind re-reviewing?

docs/adrs/006-jobs.md Outdated Show resolved Hide resolved
It works just fine, and removes the need to specify outputs up front.

- [ ] Should we allow specifying directories as outputs?
Feels globby, so it might have some of the same concerns as globs in inputs.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Let's not at first - can always revisit later!


- [ ] Should we require output files be written to a special directory instead?
Nix does this with `$out`.
It works just fine, and removes the need to specify outputs up front.
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Is the idea here that we set up file watching on that directory and detect what the output files are that way?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's just a differently-flavored opt-in mechanism for outputs. We don't need to (and it would require us to scan the FS under that directory anyway so it's maybe not for us!)

Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

(We talked about this in realtime and concluded we shouldn't do it.)

Copy link
Member Author

Choose a reason for hiding this comment

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

sync talk: don't do it, folks. Remove the TODO

Co-authored-by: Richard Feldman <oss@rtfeldman.com>
tools: [ openapiGenerator ]
command: exec "openapi-generator-cli" [ "generate", "-i", "spec.json", "-o", "api-client", "-t", "elm" ],
outputs: [ "api-client" ],
persistAt: [ "/src/api-client" ],
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Suggested change
persistAt: [ "/src/api-client" ],
persistAt: [ "src/api-client" ],

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should distinguish between these, actually! / should be the root of the project, not the root of the FS.

Copy link
Sponsor Contributor

@rtfeldman rtfeldman Oct 27, 2021

Choose a reason for hiding this comment

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

hmm, that would surprise me if I were writing this!

Like if I pasted in a hardcoded path while trying something out locally - say, /Users/rtfeldman/whatever - I think I'd be confused if it told me it couldn't find a file at that path relative to the current directory! I suspect my first instinct would be to file a bug report.

Supporting absolute paths does seem like it would indeed be a mistake though - what if we gave a helpful error message if you try to use one?

Copy link
Member Author

Choose a reason for hiding this comment

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

mm… that does make sense. Thanks, I appreciate how you're always thinking about the early experience of using tools we build!

Maybe the rule is "relative paths, always relative to the directory containing the build file." That could get annoying, though, if you've got a bunch of Roc files in a monorepo setup and want to require something (a library, maybe) across different folders. So then, maybe it's relative to the root build file? But that could get annoying for long paths.

Helpful error message does make sense, though.

@BrianHicks BrianHicks merged commit d6107bb into trunk Jul 29, 2022
@BrianHicks BrianHicks deleted the roc-api branch July 29, 2022 03:19
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.

None yet

2 participants