Expanding the patching API

[msaraiva]

Hi @doorgan!

We're gathering some folks in the community who want to extract the patcher API from Surface (which is built on top of Sourceror) into a separate project so we could have a generic higher-level library for patching Elixir projects. The lib would also include a few of the most common patchers, like some you can find here https://github.com/surface-ui/surface/blob/main/lib/mix/tasks/surface/surface.init/file_patchers/mix_exs.ex.

One of the things raised during the initial discussion was that we may need to add more functions to sourceror or maybe improve some of the existing ones so they could work on different scenarios. I remember we've discussed the possibility of expanding sourceror's patching API in the past but since it's been a while since out last conversation, I wonder if you still have plans on that regard and how do you see the evolution of Sourceror in that direction. This is important for us to know as we're willing to dedicate part of the effort to make direct contributions to Sourceror.

We've created a group on Slack to discuss the new lib and the next steps we should take. In case you want to participate in the discussion, please let us know.

Thanks for creating Sourceror! ❤️

/cc @davydog187, @zachdaniel, @paulstatezny

[doorgan]

Hi @msaraiva!

I'm glad to see that Sourceror is being of use for the community.

I wonder if you still have plans on that regard and how do you see the evolution of Sourceror in that direction.

I have no plans to expand the patching API, and the current direction is to just make sure it's stable enough and works well enough to be a building block for other tools. The reason for this is that I currently don't have the mental bandwidth to expand what Sourceror is capable of(lots of personal and mental health issues I have to take care of that make it hard for me to concentrate).

That said, however, I do welcome contributions to Sourceror(be it a new or improved patching API, new functions, bug fixes). I don't want my personal circumstances to be a roadblock for the elixir community in this space, so I'm very open to the improvements you may want to make to Sourceror. What's more, I'm even also willing to add some of you as maintainers of this repository if needed.

I'm in the Elixir Slack too as dorgan, feel free to add me to the group or channel :)

cc @NickNeck since you've been a big contributor to Sourceror and since this may also be relevant for recode

[NickNeck]

@doorgan thank you for thinking of me. @msaraiva if you interested on one more opinion, fell to add me to the channel. I'm mkr in the Elixir slack.

[zachdaniel]

A proposed API for composing many patches to a project. This is very similar and inspired by the patterns that @msaraiva
put forth with the code in surface. https://gist.github.com/zachdaniel/947bda4a8866725b5f5d118f237563b1

[NickNeck]

@zachdaniel that looks great.

In recode I have implement the structs Recode.Project and Recode.Source to represent all source files of a project. During the creation of a project the source file will be "analysed". So you can get a source for a given module with Project.module(project). The Recode.Source provides a function zipper/1 that returns a zipper for the current AST. The source can be updated with Source.update(source, ChangedBy, ast: ast). All updates will be saved in the %Source{} so we have infos for all updates. A source in the Project can be updated with Project.update(project, source). Recode can now generate a report for all changes in a project. If recode does not run in dry mode one Project.save(project) will write everything to disk. The %Source{} can also save issues or the path for the source can be updated.

I am not sure if this should be a part of sourceror. I was thinking about to bring this to it own package (project_ex).

I have also started some experiments with the modules Recode.Traverse. This modules provides functions to move the zipper to the right position. For now we have the functions to/2 and to_defmodule/2. Examples for this functions can be found at https://github.com/hrzndhrn/recode/blob/traverse/lib/recode/traverse.ex . I could imagine that as an extension in sourceror.

I have created a little script in the recode example that shows all the things above in action, see https://github.com/hrzndhrn/recode/blob/traverse/examples/my_code/scripts/add_use.exs

[zachdaniel]

Wow! So it looks like you've already implemented practically exactly the proposed API! That is huge 🔥 🔥 🔥 My 2 cents is that that logic would be best served if it lived in sourceror eventually, but I don't think that it is a requirement by any means. I do think that likely some of the extra linting logic in recode would be too much to make that the package distributed for generic code rewriting, so if you were to move that out to something like recode_linter then recode could be the package that is depended on for source code modifications?

Come to think of it, the only thing I think your API may be missing is the logic to create a file, but I imagine that isn't a complex addition to your set up.

Awesome stuff.

[zachdaniel]

@NickNeck if you did extract that project modification toolchain to its own repo, I'd likely start using it in the next couple of weeks and would happily contribute. I'd likely end up copying lots of @msaraiva's AST modification utilities from sourceror. It would be ideal to find some way to ensure proper attribution for @msaraiva's code there.

[NickNeck]

@zachdaniel

Awesome stuff.

Thank you.

Come to think of it, the only thing I think your API may be missing is the logic to create a file, but I imagine that isn't a complex addition to your set up.

I have added an example that deletes and creates sources, see https://github.com/hrzndhrn/recode/blob/traverse/examples/my_code/scripts/module_per_file.exs

I changed my mind after my last post. I think Recode.Project, Recode.Source and Recode.Traverse could be a good extension for sourceror. @doorgan what do you think?

If Recode.Project and Recode.Source goes not to sourceror I will extract it to another package. Then I just need a good name for the new package 🤔 .

[doorgan]

@NickNeck I'm still reviewing recode, as I didn't have the change to do it before

I think at least Recode.AST and Recode.Traverse are excellent candidates to be ported to Sourceror. The rationale being that in it current state Sourceror offers facilities to retrieve and update information from the AST, as well as facilities for traversing(the traverse/prewalk/postwalk and zipper), so moving and expanding the functions that answer questions about an AST piece and functions that facilitate AST traversal seems like a good first step. PRs in this direction are most welcome! may of the functions currently living in Sourceror can be moved to dedicated modules like you have done in recode.

I'm still considering Recode.Source and Recode.Project in particular, as these are higher level APIs that fulfill what if being discussed in this issue(excellent work on that library 🙂 ), but there are some aspects that may not fully align with the idea of Sourceror providing flexible building blocks.

I may be missing something, but is recode assuming that the user is running the default elixir formatter on their projects? Sourceror supports patching ranges of text to avoid having to format the whole AST and messing up user formatting in case they don't run the official formatter, so if we are going to include these higher level APIs, we'd need a way to support patching string ranges, not just changing the AST and outputting a whole new source code.

Operations on the AST don't have to worry about that, as you can just update the AST fragment, convert it to a string, and replace the original text range with the new string, but I'm not sure yet about how that would work with something like Recode.Source

Thoughts?

[NickNeck]

@doorgan I am happy that you like Recode.AST and Recode.Traverse. I will start the next days with some PRs to bring it to sourceror.

I am also fine with your opinion about Recode.Project and Recode.Source. I will extract this from recode and create the package rewrite for that stuff. @zachdaniel is that ok for you?

In the moment recode is just for users they like the Elixir formatter. Therefore, recode not only assumes that the Elixir formatter was used, but the first action of recode is to apply the Elixir formatter to the code. I have made this decision because that makes things much more simpler. I think it would be great to be able to make code changes without formatting the code. I will have a look how to implement this. I assume that might be a little tricky.
@zachdaniel the formatting of the code happens in Recode.Source. I hope that is not a problem for the things you want to do.

[zachdaniel]

🤔 I think perhaps we can just make formatting a configurable behavior, but realistically I can't imagine anyone doing what we're doing(rewriting source) without wanting at least the end result to be formatted. I think if it were totally up to me we'd place all of it in sourceror, but I'm happy either way :)

[doorgan]

@NickNeck Awesome, thank you so much :)

@zachdaniel the issue is that we need the formatter to produce proper string from a piece of AST, but we can't just assume the user wants the whole file to be formatted. This puts us in a situation where we need to decide if Sourceror needs to support these users(IIRC from previous discussions on the implementation of Sourceror.patch_string, that is what Surface needed) in the higher level API.

Of course just using the formatter for everything is the simplest solution to all problems, we may as well move everything to sourceror and commit to using the formatter for everything, while still exposing the lower level functions for the users that want to produce fine grained modifications that don't mess up their personal formatting style in the whole file. That's also an option.

[zachdaniel]

Yeah, I definitely think it makes sense to leave not formatting as an option to allow for cases where people care. I imagine it should relatively easily be added as an option to the API, perhaps even defaulting to false to protect against someone somehow messing with source that shouldn't be formatted.

[NickNeck]

I have pushed the first beta of rewrite.