-
Notifications
You must be signed in to change notification settings - Fork 102
Expand link
to handle aliases. DRY tests mechanics.
#1237
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
Conversation
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@MilesCranmer would you mind reviewing this. Thanks |
link
to handle aliaseslink
to handle aliases. DRY tests mechanics.
Resolved my comments as I put them into #1242. By the way, will this mess up any of the existing serialized files at all? I'm assuming no but I really don't have a clear enough understanding for the internals to say. |
From a design perspective I'm a little worried by how LinkedChannel and AliasChannel seem to be so different. e.g., LinkedChannel looks to be creating shell scripts, whereas AliasChannel is using runtime resolution. I don't see a strong reason for this split. I think they should use a similar mechanism. |
To be honest I think the alias solution is better than the existing LinkedChannel solution. The LinkedChannel solution seems to rely on generating shell scripts (?) which seems kinda fragile. |
Maybe if people downgrade after using this new feature? I don't think we need to worry about that, we can try it out in the prerelease. I think your other comment could be a follow on consideration? Are you happy to approve this so we can move ahead? |
I think it's probably good to go. I haven't messed around with it locally though which is something I'd like to try. Have you given it some testing in your terminal at all? I think we should try to break things a little |
We had a bug and a test gap for a general |
@IanButterworth FYI I think both of my PRs #1241 and #1242 got squashed into a single commit without me included as coauthor |
Odd. Squash merges usually retain authorship. |
I think if you do a |
I didn't edit it. Maybe the GitHub default changed it. |
Nope the website. I have an enhanced GitHub chrome extension, it could be that. I'll look when at my computer. |
Alternative to #1115
See discussion around #1115 (comment)
Closes #838
Closes #830
Closes #1115
Developed with Claude