Skip to content

make StaticArrays dependency into an extension - #23

Draft
KristofferC wants to merge 2 commits into
JuliaDiff:masterfrom
KristofferC:kc/chainrules_ext
Draft

make StaticArrays dependency into an extension#23
KristofferC wants to merge 2 commits into
JuliaDiff:masterfrom
KristofferC:kc/chainrules_ext

Conversation

@KristofferC

@KristofferC KristofferC commented Dec 29, 2022

Copy link
Copy Markdown

Shouldn't be merged until the Registrator deployment of JuliaRegistries/Registrator.jl#397 is done.

Note, this reverts #22 since there is no point of a Core package with an extension. This means that people on 1.8 will get the full StaticArray load time but I think that doesn't really matter since ForwardDiff loads it anyway as it is right now.

@codecov-commenter

codecov-commenter commented Dec 29, 2022

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.84%. Comparing base (276d1d7) to head (0a4cdba).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #23   +/-   ##
=======================================
  Coverage   86.84%   86.84%           
=======================================
  Files           1        2    +1     
  Lines          76       76           
=======================================
  Hits           66       66           
  Misses         10       10           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@devmotion devmotion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Based on JuliaArrays/StaticArraysCore.jl#15 and the recently added weak dependency on StaticArrays in ForwardDiff, I think we should make StaticArrays a weak dependency here as well instead of depending on StaticArraysCore.

Comment thread Project.toml
name = "DiffResults"
uuid = "163ba53b-c6d8-5494-b064-1a9d43ac40c5"
version = "1.1.0"
version = "1.0.3"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This line should not be reverted, I think:

Suggested change
version = "1.0.3"
version = "1.1.0"

Comment thread Project.toml
StaticArrays = "90137ffa-7385-5640-81b9-e52037218182"

[extensions]
StaticArraysExt = "StaticArrays"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be good to use a prefix?

Suggested change
StaticArraysExt = "StaticArrays"
DiffResultsStaticArraysExt = "StaticArrays"

Comment thread ext/StaticArraysExt.jl
@@ -0,0 +1,25 @@
module StaticArraysExt

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
module StaticArraysExt
module DiffResultsStaticArraysExt

Comment thread src/DiffResults.jl
Base.show(io::IO, r::MutableDiffResult) = print(io, "MutableDiffResult($(r.value), $(r.derivs))")

if !isdefined(Base, :get_extension)
include("../ext/StaticArraysExt.jl")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
include("../ext/StaticArraysExt.jl")
include("../ext/DiffResultsStaticArraysExt.jl")

@gdalle

gdalle commented Mar 15, 2024

Copy link
Copy Markdown
Member

What would you think about #28 instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants