OxygenFramework / Oxygen.jl

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

refactor code to load modules normally #160

Closed ericphanson closed 7 months ago

ericphanson commented 7 months ago

I noticed when developing https://github.com/OxygenFramework/Oxygen.jl/pull/158 the tests showed a lot of messages like

WARNING: Method definition get(Function, String) in module Core at /Users/eph/Oxygen.jl/src/core.jl:504 overwritten in module Core on the same line (check for duplicate calls to `include`).
WARNING: Method definition get(Function, Function) in module Core at /Users/eph/Oxygen.jl/src/core.jl:505 overwritten in module Core on the same line (check for duplicate calls to `include`).

This fixes some of them by loading the module once.

A rule of thumb in Julia is each file should be includeed exactly once. Each time you include a module, it creates a separate copy of it. Instead, we should use relative using statements like using ..Util. See this section of the Julia manual for more.

I suspect some of this is to facillitate a testing workflow. I wonder if perhaps you aren't using Revise, and thus reinclude the source code to reload it?

I would highly recommend Revise for this, it is even mentioned in the main Julia manual: https://docs.julialang.org/en/v1/manual/workflow-tips/#Revise-based-workflows.

ericphanson commented 7 months ago

I see https://github.com/OxygenFramework/Oxygen.jl/pull/158 does some of this refactoring as well. This PR just refactors the includes to usings without touching anything else, so I think it could make sense alone, and adding features like #158 could be built on top of it.

codecov[bot] commented 7 months ago

Codecov Report

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

Comparison is base (8fed3b9) 98.31% compared to head (d19dcee) 98.59%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #160 +/- ## ========================================== + Coverage 98.31% 98.59% +0.27% ========================================== Files 14 14 Lines 1070 1068 -2 ========================================== + Hits 1052 1053 +1 + Misses 18 15 -3 ```

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

JanisErdmanis commented 7 months ago

This is pretty much covered in #158 refactor. I also added show_errors keyword to serve and added suppression of errors at the call site on internalrequest which expects errors.

ericphanson commented 7 months ago

This is pretty much covered in https://github.com/OxygenFramework/Oxygen.jl/pull/158 refactor

That PR has a 1k line diff though, which generally is harder to review and merge than an incremental improvement

I also added show_errors keyword to serve and added suppression of errors at the call site on internalrequest which expects errors.

I think using @test_logs as in #161 is a better approach than @suppress, because it additionally tests that those log messages are indeed being emitted as expected.

JanisErdmanis commented 7 months ago

That PR has a 1k line diff though, which generally is harder to review and merge than an incremental improvement

The refactor was incremental, and all tests have been working since Saturday. It was just not my call to merge. The changes you have proposed will be useful and will be reviewed and incorporated after the merge.

I think using @test_logs as in https://github.com/OxygenFramework/Oxygen.jl/pull/161 is a better approach than @suppress, because it additionally tests that those log messages are indeed being emitted as expected.

I agree.

ndortega commented 7 months ago

Sorry about the bad timing, most of these changes have already been addressed in the previous merge request. I plan on closing this PR for now simply due to the number of merge conflicts to resolve. I'll make sure to fix the imports in the demo files in the next pr.