Conversation
|
@workingjubilee opinions? |
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
kinnison
force-pushed
the
ensure-env-file
branch
from
2c3d739 to
d33c53f
Compare
|
This appears to be sane to me, and I need to re-prepare 1.23.1 so I'm merging it. |
| // Nothing to do for Windows for now | ||
| pub fn do_write_env_files() -> Result<()> { | ||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a literally useless function i.e. it does nothing except further enmesh the two OS' APIs.
I submitted 3 PRs with the express intention in each one of reducing the frequency of these in the codebase so that Linux and Windows devs would not have to consult enmeshed OS APIs, and I was hardly given even 24 hours to review this, whereas at least one of my PRs was held up for weeks in review for not being pretty enough or not using APIs that had no explanation for their usage.
There was a problem hiding this comment.
This is a fair assessment. It does increase coupling in an API you worked very hard to decouple. I can only apologise for the timing and the presentation of this change, I was working under (perhaps imagined) pressure to get a fix out for users because 1.23.0 broke a behaviour people were reasonably relying on and it had broken a number of CIs (not just this, but also the macos 10.13 problem).
I will work on a PR to decouple this again, I apologise for rushing through something less than ideal.
2 participants
This should fix #2578