firrtl icon indicating copy to clipboard operation
firrtl copied to clipboard
verified publisher icon chipsalliance

[deprecation clean up] remove firrtl.stage.transforms.CheckScalaVersion.

Open sequencer opened this issue 4 years ago • 5 comments

Contributor Checklist

  • [ ] Did you add Scaladoc to every public function/method?
  • [ ] Did you update the FIRRTL spec to include every new feature/behavior?
  • [ ] Did you add at least one test demonstrating the PR?
  • [ ] Did you delete any extraneous printlns/debugging code?
  • [ ] Did you specify the type of improvement?
  • [ ] Did you state the API impact?
  • [ ] Did you specify the code generation impact?
  • [ ] Did you request a desired merge strategy?
  • [ ] Did you add text to be included in the Release Notes for this change?

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

  • [ ] Did you add the appropriate labels?
  • [ ] Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • [ ] Did you review?
  • [ ] Did you check whether all relevant Contributor checkboxes have been checked?
  • [ ] Did you mark as Please Merge?

sequencer avatar Nov 21 '21 09:11 sequencer

We have dropped 2.11 in 1.5, maybe this is a dead code since 1.5?

sequencer avatar Nov 23 '21 00:11 sequencer

We have dropped 2.11 in 1.5, maybe this is a dead code since 1.5?

It is dead code in 1.5, but it wasn't dead in 1.4. Thus if someone were referring to this code in their 1.4 code, upgrading to 1.5 will just break it. Thus instead we deprecate in 1.5 and remove in 1.6.

jackkoenig avatar Nov 30 '21 03:11 jackkoenig

It is dead code in 1.5, but it wasn't dead in 1.4. Thus if someone were referring to this code in their 1.4 code, upgrading to 1.5 will just break it. Thus instead we deprecate in 1.5 and remove in 1.6.

Got it! So in this PR, I think we can delete references to CheckScalaVersion, and keep CheckScalaVersion object as a user API?

sequencer avatar Nov 30 '21 03:11 sequencer

Another idea is back port deprecation to 1.4 branch and remove this code in master, will this be OK?

sequencer avatar Nov 30 '21 03:11 sequencer

Got it! So in this PR, I think we can delete references to CheckScalaVersion, and keep CheckScalaVersion object as a user API?

We should just keep the deprecated API as is in 1.5 and as soon as 1.5.0 is released we can merge this PR for release in 1.6

Another idea is back port deprecation to 1.4 branch and remove this code in master, will this be OK?

No, because the code isn't dead in 1.4, it's perfectly legal for a user to being using it so they shouldn't see deprecation warnings.

jackkoenig avatar Nov 30 '21 18:11 jackkoenig