phanrahan / magma

magma circuits
Other
253 stars 24 forks source link

[v3.0.0] Export v2 objects as core objects #1342

Closed rsetaluri closed 11 months ago

rsetaluri commented 11 months ago

Exports Generator2 as Generator, inline_verilog2 as inline_verilog and bind2 as bind.

For Generator and inline_verilog, both versions still exist internally, but this is a breaking change with respect to the external API. For example, m.Generator is now what m.Generator2 used to be, which has a different interface.

For bind, Circuit.bind and Generator.bind no longer exist, and only m.bind remains. However, the internal code backing Circuit.bind still exists.

Additionally, some tests have been deleted or skipped for convenience since they cover non-public API (previously deprecated).

A future change should purge the internal remnants of the old versions. However, such a change will not require a major upgrade, since the external API will not break.

rsetaluri commented 11 months ago

LGTM, minor nits on import (maybe using as is better since it forces the renaming to be inline with the import so it doesn't get lost)

This is a good point, but actually a lot of internal code still uses the <name>2 versions. I figure this can be an internal only change after the fact? And have duplicates exposed.

rsetaluri commented 11 months ago

LGTM, minor nits on import (maybe using as is better since it forces the renaming to be inline with the import so it doesn't get lost)

This is a good point, but actually a lot of internal code still uses the <name>2 versions. I figure this can be an internal only change after the fact? And have duplicates exposed.

I'll add comments to make this clear.

codecov-commenter commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (277d23a) 86.06% compared to head (b463ffa) 84.97%.

:exclamation: Current head b463ffa differs from pull request most recent head 9844d70. Consider uploading reports for the commit 9844d70 to get more accurate results

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1342 +/- ## ========================================== - Coverage 86.06% 84.97% -1.09% ========================================== Files 171 171 Lines 17922 17916 -6 ========================================== - Hits 15424 15224 -200 - Misses 2498 2692 +194 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.