Use Base.@kwdef

[prbzrg]

https://docs.julialang.org/en/v1/base/base/#Base.@kwdef

[ai-maintainer]

AI-Maintainer Review for PR - Use Base.@kwdef

Title and Description 👍

Title and Description

The title of the pull request is clear and concise. It effectively communicates the main change, which is the use of `Base.@kwdef`. However, the description could be improved. While the link to the documentation is helpful, it would be beneficial to include a brief explanation of why and how `Base.@kwdef` is being used in this context.

Scope of Changes 👍

Scope of Changes

The changes in this pull request are narrowly focused on using the `Base.@kwdef` macro. The author is not trying to resolve multiple issues simultaneously, which is good practice as it keeps the scope of the PR limited and easier to review.

Testing ⚠️

Testing

The description does not provide information on how the changes were tested. It's important to include this information to ensure the changes don't introduce new bugs and work as expected. Please provide details on the testing approach used.

Code Documentation 👍

Code Documentation

All the newly added functions, classes, or methods have docstrings describing their behavior, arguments, and return values. This is good practice as it makes the code easier to understand for other developers.

Suggested Changes

Please update the PR description to include a brief explanation of why and how Base.@kwdef is being used in this context. Also, provide details on the testing approach used to verify the changes.

Reviewed with AI Maintainer

[ai-maintainer]

AI-Maintainer Review for PR - Use Base.@kwdef

Title and Description 👍

The Title and Description are concise

The title and description of the pull request are concise. They indicate that the changes involve the use of the Base.@kwdef macro. However, the description could be more explicit in explaining how Base.@kwdef is being used in the code changes. It would be beneficial to provide a brief explanation of why this change is beneficial or necessary.

Scope of Changes 👍

The changes are narrowly focused

The changes are specific to implementing Base.@kwdef in various structs and updating the corresponding tests. It does not seem like the author is trying to resolve multiple issues simultaneously.

Testing ⚠️

Testing methodology is not described

The description does not describe how the author tested the changes. It would be helpful if the author could provide information on the testing methodology or approach used to verify the changes.

Documentation 👍

All new functions, classes, or methods have docstrings

All newly added functions, classes, or methods have docstrings describing their behavior, arguments, and return values. There are no functions, classes, or methods in the diff that need docstrings.

Suggested Changes

• Please provide a brief explanation in the description of how Base.@kwdef is being used in the code changes and why this change is beneficial or necessary.
• Please provide information on the testing methodology or approach used to verify the changes.

Reviewed with AI Maintainer

[prbzrg]

I tested it locally, tests passed.

(ADTypes) pkg> test
     Testing ADTypes
      Status `C:\Users\Hossein Pourbozorg\AppData\Local\Temp\jl_qjqKnd\Project.toml`
  [47edcb42] ADTypes v0.1.6 `C:\Users\Hossein Pourbozorg\Code Projects\github-repos\ADTypes.jl`
  [8dfed614] Test `@stdlib/Test`
      Status `C:\Users\Hossein Pourbozorg\AppData\Local\Temp\jl_qjqKnd\Manifest.toml`
  [47edcb42] ADTypes v0.1.6 `C:\Users\Hossein Pourbozorg\Code Projects\github-repos\ADTypes.jl`
  [2a0f44e3] Base64 `@stdlib/Base64`
  [b77e0a4c] InteractiveUtils `@stdlib/InteractiveUtils`
  [56ddb016] Logging `@stdlib/Logging`
  [d6f4376e] Markdown `@stdlib/Markdown`
  [9a3f8284] Random `@stdlib/Random`
  [ea8e919c] SHA v0.7.0 `@stdlib/SHA`
  [9e88b42a] Serialization `@stdlib/Serialization`
  [8dfed614] Test `@stdlib/Test`
     Testing Running tests...
Test Summary: | Pass  Total  Time
ADTypes.jl    |   43     43  0.2s
     Testing ADTypes tests passed

julia> versioninfo()
Julia Version 1.9.2
Commit e4ee485e90 (2023-07-05 09:39 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: 12 × Intel(R) Core(TM) i7-10750H CPU @ 2.60GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-14.0.6 (ORCJIT, skylake)
  Threads: 13 on 12 virtual cores

Why do they fail here?

[Vaibhavdixit02]

That's because the ci script has 1.0 instead of 1 and hence a julia 1.0.x is getting used. I have updated that in #15 but maybe you can do it here, one of us will have to rebase and update 😛

[prbzrg]

Oh, now I see it 😁. I updated the CI file.

[Vaibhavdixit02]

I am curious though why do you think we should change to this instead of the constructors we have right now?

[prbzrg]

IMO, using a helper macro makes the code easier to read; and since Julia 1.9 @kwdef is an exported macro. I guess because it's popular!
Performance-wise, it literally translates to the same thing. So there is no difference :

julia> @macroexpand Base.@kwdef struct AutoReverseDiff <: AbstractADType
           compile::Bool = false
       end
quote
    #= util.jl:589 =#
    begin
        $(Expr(:meta, :doc))
        struct AutoReverseDiff <: AbstractADType
            #= REPL[2]:2 =#
            compile::Bool
        end
    end
    #= util.jl:590 =#
    AutoReverseDiff(; compile = false) = begin
            #= util.jl:567 =#
            AutoReverseDiff(compile)
        end
end

[Vaibhavdixit02]

Cool, thanks for the response! I have used it in the past in another package but that was a while back, and haven't used it since hence just wanted your thought behind this

[devmotion]

This PR broke the package on Julia < 1.6 < 1.1. IMO it's fine to use Base.@kwdef but the problem is that the Julia compat entry was not updated.

[prbzrg]

@kwdef was added to v1.1 based on docs and v1.1 release notes
CI doesn't test Julia < 1.6, so if we want them, we should add them to be sure we don't break on them.

[devmotion]

As I said, I'm fine with supporting only Julia >= 1.6. The problem with this PR is that it disabled CI for 1.0 (since tests failed) but did not adjust the compat entry accordingly. This broke at least Julia 1.0.

[prbzrg]

Oh, now I see it. Thanks for noticing it.