-
-
Notifications
You must be signed in to change notification settings - Fork 287
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
Brewfile.lock.json
's filename is misleading
#1188
Comments
We've had this feedback a few times and are still considering how best to address it without breaking existing usage. |
i'm personally in favour of |
Yes, that approach would make sense to me @jacobbednarz. |
The migration path sounds good. Should we also offer the user the chance to add it to the As for the naming of the file, I would argue that I'd mentally frame it as a log or report of the previous run, hence my suggestion – it captures what happened when we ran |
Not sure I understand the question @boardfish, can you elaborate?
I think this is definitely less obvious than Also perhaps worth noting that the implementation that never really got done here but could/should be as part of this work: on a failure, diff what would be written to a new
Agreed. Once we flesh this out: if you're game to create a PR @boardfish, we'd love that! I know you're a talented Ruby developer and I'm happy to help get any draft/WIP PR along to completion. Thanks for your thoughtful responses and your own OSS work ❤️ ! |
I'll break down what I'm guessing this might look like in practice, since the experience from start to finish sort of plays into where and when we do it. It might not be apparent to the user that the
We might log something here to the effect of:
Since folks probably don't want to commit these files, we might then prompt:
...if one exists. Some things we might be concerned with that might make this tougher or less viable are:
It's a small thing that might help folks with the migration and make them more aware of it, but those concerns and how they might differ across environments might make it a heavier lift. |
👍🏻 this makes sense (but ideally without the "if" because we can print a different message depending).
👎🏻 I think we should leave this up to the docs to inform. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
I'd like to see this. I like I like writing to that file if nothing exists or writing to I don't like altering files for users unless the program completely "owns" that file, so suggesting they take a manual action will have to suffice. I like more actively advising the user that the file can be ignored, perhaps with a nudge to add to .gitignore or to suppress it entirely with I also like setting deprecation and abiding by it, so the No matter what, I want the new file to have a version number, e.g., what I did in #1175, but perhaps without the other things I did that made that PR unsuitable. Other ideas while we're considering a renameOne idea I'd had at some point was to switch from JSON to JSONL and just append the new run's state. Then the user can ingest the file through something that looks at the differences per object instead of having to spelunk through git history. There might be some storage-related pros and cons of whole file replacement vs. appending a line, but the impact is so small that I'm not interested in finding out how many bytes difference it'd be. My problem with JSONL is versioning it, something I want to do. Another idea I had was not to use JSON but rather a simpler key-value output that would probably just look like gron'd JSON. That'd be easier for a human to act and consume with ye olde |
I disagree: we should diff with the previous version and present that output to users.
This is already documented?
Really don't see a need for this. Homebrew Bundle is rolling release and I've seen no evidence of this file having external consumers.
This feels like overkill, sorry.
Given the data is hierarchical: I don't think optimising for |
Sorry, my remark was ambiguous. I don't want
It is, but the traffic asking about indicates that we could do better informing the user more actively about how
The verbosity of what I'd really like to see might be too much:
I've never regretted having a version number in a data dump, but I have regretted not putting one in until it would be useful. We're talking about migrating from one filename convention (effectively v1) to another (effectively v2) for a filename that is not guaranteed.
Probably, given how there's
so I'll drop that idea.
Yeah, it's probably unnecessarily complex for this use case. Another idea: What if this file was written to
The eliminates checking it in, though, for people who want to do that. |
I like the idea of putting the logfile in a less conspicuous location that Homebrew is responsible for, that's a great shout! |
👍🏻
I don't think the traffic warrants outputting this message for every user every time. If we did it once on e.g. first creation: 👍🏻
I've definitely regretted merging PRs with such things only for them to go unused and cause unneeded bloat and confusion on reading the code later.
❤️
Please no 😁 That's making it even less intuitive/expected than it is currently. Let's not blow the scope up on this one. We've got enough decided here for someone to open a PR if desired. If we want to iterate beyond that: let's do so when the initial issue is resolved. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
Here's an interesting use of these lock files: https://github.com/advanced-security/brew-dependency-submission-action |
Shouldn't be committed, see Homebrew/homebrew-bundle#1188
Why is this change needed? -------------------------- This appears to be a generated state file, particular to the device. How does it address the issue? ------------------------------ Ignore the generated file. Any links to any relevant tickets, articles, or other resources? --------------------------------------------------------------- Homebrew/homebrew-bundle#1188 Did you complete all of the following? -------------------------------------- - Run test suite? - Add new tests? - Consider security implications and practices?
#552 seems to suggest that the
Brewfile.lock.json
file generated bybrew bundle install
is for debugging purposes. However, the name suggests to me that it is responsible for ensuring thatbrew bundle install
has the same results for different users in the same way that, for example,Gemfile.lock
does against Ruby'sbundler
gem. With aGemfile.lock
in place,bundle install
will attempt to install the same versions of the specified gems across two different machines. Likewise,Brewfile.lock.json
's name (alongside this extension's intentional similarity tobundler
) suggests to me that this file is responsible for the same thing. (There's a bit of oversimplification there for the sake of argument, but you get my gist.)It came up in code review just now that perhaps we should add the filename to our
.gitignore
, and I had to seek out that PR to confirm that for myself. However, this could be avoided for folks in future if the generated file has a different name. The real responsibility of the file is for debug logging, so maybebrew-bundle-debug-log-YYMMDDHHMM.json
would be a good idea.Whatever we go with here, it might also be good to get it merged into GitHub's own macOS gitignore, along with
Brewfile.lock.json
.The text was updated successfully, but these errors were encountered: