Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No 'reload' argument to 'detect_macros' #2

Closed
samanklesaria opened this issue Feb 7, 2020 · 4 comments
Closed

No 'reload' argument to 'detect_macros' #2

samanklesaria opened this issue Feb 7, 2020 · 4 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@samanklesaria
Copy link
Contributor

It seems that macropy from https://github.com/lihaoyi/macropy defines detect_macros without a reload argument. Loading the imacropy extension fails, as the file imacropy/console.py calls detect_macros with a reload argument. This argument should be removed.

@samanklesaria samanklesaria changed the title No reload argument to detect_macros No 'reload' argument to 'detect_macros' Feb 7, 2020
samanklesaria added a commit to samanklesaria/imacropy that referenced this issue Feb 7, 2020
@Technologicat
Copy link
Owner

Thanks for the report, and #3!

I have used @azazel75's fork of MacroPy, because a while ago (IIRC, late 2018), it was the official version for Python 3.

But it seems things have changed in the meantime so that azazel75 now has commit access to @lihaoyi's original repository, so maybe I should switch to that version and begin testing against that.

I'll investigate a bit, and report here. I'll most likely merge your PR when done.

@Technologicat
Copy link
Owner

Technologicat commented Feb 9, 2020

Aaaah, turns out that reload thing is mine! I'd totally forgotten about it, fortunately we have git blame! Technologicat/macropy@295372c

This small change to MacroPy was part of my original PR to include imacropy-like functionality into MacroPy itself, before we decided that this functionality is better provided as a separate package maintained by me.

It enables imacropy to live-reload macro definitions. Without it, changes to macro definitions on disk made during an imacropy session will not take effect, even if one re-imports the affected macros into the REPL.

I suppose the solution is to disable it for now, and think a bit. There are two possible long-term solutions:

  1. I could submit a new small PR to @azazel75, requesting to add just the reload parameter to support this interactive use case. I'm not sure, it's probably too specific to be acceptable for MacroPy in general.

  2. It could just be documented, in imacropy, that by the principle of least surprise, imacropy performs no module reloading.

    So, if you want to update macro definitions that have been modified on disk during an imacropy REPL session, then - just like with any regular Python code - you must importlib.reload(thatmodule) to refresh the module itself, before issuing the command from thatmodule import macros, ... to load the new macro definitions from that module into the REPL.

I'll mull over it for a couple more days, and then conclude this.

@Technologicat Technologicat self-assigned this Feb 9, 2020
@Technologicat Technologicat added the bug Something isn't working label Feb 9, 2020
@Technologicat Technologicat added this to the 0.2.0 milestone Feb 9, 2020
@Technologicat
Copy link
Owner

Hmm, there's also this option:

  1. Duplicate the import resolution logic from macropy.core.macros.detect_macros, and reload the affected modules ourselves before calling detect_macros.

Easy to explain, may be a good idea...

Technologicat added a commit that referenced this issue Feb 10, 2020
@Technologicat
Copy link
Owner

Went for option #3. We only need the module names and that's just a couple lines of code.

Live reloading should work with a stock MacroPy now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants