remove the interface to support non .jl files #921
Conversation
|
No, I'm not a fan of this change. Besides the multi-registration issue, reliance on init, there's also the issue that there's no guarantee that you can map from an extension to the parser for the file. It needs to be tied into whatever mechanism is used for code loading of non-julia frontends (Cedar used include with a custom struct, which is why it's set up this way). It's not a Revise specific mechanism. That said if you need to drop the feature for now, I don't think anybody is actively using it and we can put it back in a few months when people are. |
aviatesk
force-pushed
the
avi/simple-non-jl
branch
from
edc4d19 to
1cf0945
Compare
The interface added in #680 for supporting non-.jl files assumes usages of custom struct, which requires overloading functions like `abspath` and is therefore quite cumbersome. Worse still, that makes it harder to refactor Revise code base, since now we need to think about the possibility of arbitrary types in places that originally needed to handle `AbstractString`. This commit provides a simpler interface: support custom files by registering parsers per file extension in `Revise.parsers::Dict{String,Any}`. The greatly simplified `non_jl_test.jl` demonstrates how much more straightforward this approach is. Of course, a potential downside is that if multiple packages register different parsers for the same extension, conflicts could arise, but since this feature is experimental, I believe we don’t need to account for that scenario yet.
aviatesk
force-pushed
the
avi/simple-non-jl
branch
from
1cf0945 to
2f11cda
Compare
|
Okay, I understand your concern. getparser(::Val{Symbol(".jl")}) = jl_parser
getparser(::Val{Symbol(".c")}) = ...Would an interface like this be better? (Though I think we might need some hacks to minimize invalidation.)
If that's the case, I'd like to suggest we merge it in this incomplete but simple state for now, and add proper custom file support after the refactor is done. |
|
It's marginally better, but has much of the same problem that it's not integrated with code loading. If the existing interface can't be supported, then remove it. There is no point in creating a replacement that nobody will use. |
|
I see, so to support code loading via e.g. |
Remove interface for supporting non-.jl files introduced in #680. This interface forces functions that used to handle only `AbstractString` to deal with arbitrary types, making refactoring across the Revise.jl codebase difficult. The simplified interface proposed in the previous commit is also fragile since it relies on initialization and runtime-mutable data structures, and so it is therefore not a complete solution. After discussion, and since this experimental interface has effectively no users, we remove it for now to enable codebase refactoring until it becomes necessary again.
aviatesk
changed the title
|
xref JuliaDebug/CodeTracking.jl#144. I also have a PR ready to go here which would leverage it. |
The interface added in #680 for supporting non-.jl files assumes usages of custom struct, which requires overloading functions like
abspathand is therefore quite cumbersome. Worse still, that makes it harder to refactor Revise code base, since now we need to think about the possibility of arbitrary types in places that originally needed to handleAbstractString.This commit provides a simpler interface: support custom files by registering parsers per file extension in
Revise.parsers::Dict{String,Any}.The greatly simplified
non_jl_test.jldemonstrates how much more straightforward this approach is.Of course, a potential downside is that if multiple packages register different parsers for the same extension, conflicts could arise, but since this feature is experimental, I believe we don’t need to account for that scenario yet.
/cc @Keno @oscardssmith Are you fine with this change? I'm currently working on some structural refactoring on Revise code base to support LS usecase, and I'd like to clean up the code base if possible before making massive changes on that.