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

scripts: west_commands: runners: remove deprecated options #52662

Merged
merged 1 commit into from Nov 30, 2022

Conversation

gmarull
Copy link
Member

@gmarull gmarull commented Nov 30, 2022

The --snr (nrfjprog) --id (jlink) and --board-id (pyocd) options were
deprecated a long time ago in favor of --dev-id. It is time to remove
them.

Ref. 5ee719e

Signed-off-by: Gerard Marull-Paretas gerard.marull@nordicsemi.no

carlescufi
carlescufi previously approved these changes Nov 30, 2022
nordicjm
nordicjm previously approved these changes Nov 30, 2022
The --snr (nrfjprog) --id (jlink) and --board-id (pyocd) options were
deprecated a long time ago in favor of --dev-id. It is time to remove
them.

Signed-off-by: Gerard Marull-Paretas <gerard.marull@nordicsemi.no>
@gmarull gmarull dismissed stale reviews from nordicjm and carlescufi via 2b6c6dc November 30, 2022 10:32
@gmarull gmarull requested a review from nashif as a code owner November 30, 2022 10:32
@gmarull gmarull changed the title scripts: west_commands: runners: nrfjprog: drop deprecated --snr scripts: west_commands: runners: remove deprecated options Nov 30, 2022
@zephyrbot zephyrbot added the area: Twister Twister label Nov 30, 2022
args.extend(['--snr', TEST_OVR_SNR])
args.extend(['--dev-id', TEST_OVR_SNR])
Copy link
Member

Choose a reason for hiding this comment

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

should update all the other references to --snr in this file.

Copy link
Member Author

@gmarull gmarull Nov 30, 2022

Choose a reason for hiding this comment

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

nrfjprog uses --snr. let me tweak the test to make things more clear.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, after checking I'm not sure removing SNR improves the code, as nrfjprog uses --snr and we're testing nrfjprog is run with the correct serial number, obtained from dev-id.

@carlescufi carlescufi merged commit 2cee5ff into zephyrproject-rtos:main Nov 30, 2022
@gmarull gmarull deleted the nrfjprog-del-snr branch November 30, 2022 15:20
@mbolivar-nordic
Copy link
Contributor

@gmarull I am not going to propose a revert, but for the record I would not have wanted to merge this PR.

Just because we can remove deprecated features does not mean we should in all cases. In cases where the maintenance burden is minimal, the benefit of preserving backwards compatibility should not be ignored. I think this is such a case.

@gmarull
Copy link
Member Author

gmarull commented Dec 5, 2022

@gmarull I am not going to propose a revert, but for the record I would not have wanted to merge this PR.

Just because we can remove deprecated features does not mean we should in all cases. In cases where the maintenance burden is minimal, the benefit of preserving backwards compatibility should not be ignored. I think this is such a case.

Users had 1y to migrate from --snr to --dev-id, I think it's enough time. I also think every deprecated feature should have a removal timeline, like "Scheduled for removal in Zeephyr X.Y".

@mbolivar-nordic
Copy link
Contributor

I don't agree with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Twister Twister area: West West utility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants