OxygenFramework / Oxygen.jl

💨 A breath of fresh air for programming web apps in Julia
https://oxygenframework.github.io/Oxygen.jl/
MIT License
383 stars 25 forks source link

Improve get() method shadowing #182

Open ndortega opened 2 months ago

ndortega commented 2 months ago
codecov[bot] commented 2 months ago

Codecov Report

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

Project coverage is 99.36%. Comparing base (82fdc0e) to head (d741ba5).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #182 +/- ## ======================================= Coverage 99.36% 99.36% ======================================= Files 24 24 Lines 1266 1267 +1 ======================================= + Hits 1258 1259 +1 Misses 8 8 ```

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

ndortega commented 2 months ago

@JanisErdmanis Here's the changes you brought up

JanisErdmanis commented 2 months ago

The test set looks good. Perhaps it could be improved with a case when package which depends on Oxygen is loaded as a module to check if there are any precompilation errors. I think that can be automated by creating a dummy package which can be put within a test directory and added to a LOAD_PATH which would then be treated as ordinary package with precompilation.

JanisErdmanis commented 2 months ago

@ndortega can we also add a method:

get(collection, key) = Base.get(collection, key)

Since users or package developers may want to overload get method this way. Perhaps even safer strategy here is:

get(args...) = Base.get(args...)
get(args...; kwargs...) = Base.get(args...; kwargs...)
JanisErdmanis commented 2 months ago

We need to comment out in Oxygen.jl with this pull request:

import Base: get

Because otherwise we have

import Base: get
get(x, y, z) = Base.get(x, y, z)

and thus if method is not found it would form a recursion. The removal of import Base: get is also good as that eliminates type piracy.

An alternative is putting the redirection get methods within the @oxidise macro itself.