Deprecate aliased commands - #2214
Conversation
There was a problem hiding this comment.
I think a smoother approach would be to continue to recognize the aliases but just exit after telling the user the replacement command (instead of executing the replacement command). This can help retrain users by telling them the new command.
Then in a later release, we can just remove all handling of the old aliases.
|
@bjhargrave for each aliased command though we have had a warning that these would be removed in 0.19. I feel like enough warning has been given that warrants deletion of the code we no longer want to maintain. Open to what others think on this though! |
I think my suggestion conforms with removing them since they no longer do anything useful and just emit a message with the proper command to use. My suggested is just a baby step to removing them by nudging the users to the proper command. I still see many people using the old command forms since they are not forced to use the new forms. (I am not even sure they read the message :-) Just removing them without a nudge to the new command is harsher than it needs to be. |
@bjhargrave We basically have been doing this except we don't exit - just warn. The issue called for removing all handling of the old aliases, but I thought adding the transitional class with the new deprecation warning was a good compromise. |
@bjhargrave I could do this though! I'll see what others think as well |
ba8532b to
3b6c23e
Compare
352070d to
5dee11f
Compare
|
Example output running ilab and then trying to run a removed command: |
leseb
left a comment
There was a problem hiding this comment.
Test files need to be updated too:
- tests/test_lab_config.py
- tests/test_lab.py
Thanks!
I think the help output should not mention the aliases. But if the user uses an old alias, the error message should state the replacement command to nudge the user. |
6a672e9 to
5dee11f
Compare
5dee11f to
b37b1b0
Compare
cdoern
left a comment
There was a problem hiding this comment.
looks good. Thank you for doing this! if all the tests are happy and the key aliases are gone, I am happy 🙏
leseb
left a comment
There was a problem hiding this comment.
One last thing, could you please update the CHANGELOG to reflect that the aliases are gone? Thanks!
d9af338 to
dbe0348
Compare
dbe0348 to
711d36c
Compare
@leseb Updated the changelog, PTAL! |
711d36c to
c1b9a98
Compare
c1b9a98 to
02e56e5
Compare
02e56e5 to
a0caf2e
Compare
a0caf2e to
ed0d995
Compare
This change removes a majority of the aliased commands, except `chat`, `generate`, `serve`, and `train`. When a user attempts to use an alias that was removed, it errors out. If a user types an alias that was kept, it proceeds with the command. Also updates the relevant test files. Signed-off-by: Alina Ryan <aliryan@redhat.com>
ed0d995 to
bbaa54b
Compare
) Updated the README file to remove mentions of aliases that [have been removed](#2214) in the latest updates. I have also fixed the formatting to reflect the current output of the `ilab` command. **Checklist:** - [x] **Commit Message Formatting**: Commit titles and messages follow guidelines in the [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/#summary). - [ ] [Changelog](https://github.com/instructlab/instructlab/blob/main/CHANGELOG.md) updated with breaking and/or notable changes for the next minor release. - [x] Documentation has been updated, if necessary. - [ ] Unit tests have been added, if necessary. - [ ] Functional tests have been added, if necessary. - [ ] E2E Workflow tests have been added, if necessary. Approved-by: bjhargrave Approved-by: nathan-weinberg
This change removes a majority of the aliased commands, except
chat,generate,serve, andtrain. When a user attempts to use an aliasthat was removed, it errors out. If a user types an alias that was kept,
it proceeds with the command.
Also updates the relevant test files.
Checklist:
conventional commits.