Implement the complete solution for processing the flat configuration structure

[daniilsapa]

Resolves #82

Hi @illright

I know it's huge, but it didn't make any sense to partition this work, because it was kind of bringing all that we've done together.

To make it a little easier for you to review, I've created a diagram of the config parsing and application flow:
(you can find types that the processes pass to each other in steiger/src/models/config/types.ts)

Notes

The function that I wrote for glob matching did not fully suit our needs because it removed nodes from the tree. And as I saw we needed more of a thing that would mark nodes instead of removing them because we’ll need to run globs on a tree several times e.g. for the following config. We need to disable /shared first and then re-enable it (yeah, we discussed that it’s a weird config, but it still could be specified). So, I decided that we needed to mark/remark nodes for every set of globs provided for a rule, and only when all sets are processed we can safely remove nodes that remained “off”

const config = [
    ...fsd.configs.recommended,
    {
      files: ['./src/shared/**'],
      rules: {
        'fsd/public-api': 'off',
      },
    },
    {
      files: ['./src/shared/**'],
      rules: {
        'fsd/public-api': 'error',
      },
    },
],

I installed ramda library. It has a lot of cool functional utils. One of the most useful functions is pipe which allows you to run operations on an array with one traversal, like filtering and mapping in the example below. There aren't many places where it's used now, but we can keep adding more in the future. It should improve the performance a bit by removing the need to re-traverse arrays with chaining array methods

[illright]

Thanks for this! I haven't finished reviewing yet, I just wanted to drop some early comments about some runtime specifics that I think are important. I'm also not sure I understand what a RuleRunEnvironment is, could you add a docstring that describes what this object represents?

[illright]

I also have one high-level architectural concern for this PR. It introduces a lot of functionality and responsibility to the config model, and it's hard for me to wrap my head around it all. I'd prefer if the config model was only responsible for reading the config and answering questions about it in a format that's convenient for Steiger itself. Then the logic of applying globs could be extracted to a feature, like it used to be before this PR.

I tried to imagine the flow of communication that would keep the config model as slim as possible, and here's what I came up with. This diagram is an imaginary dialog between Steiger (the app.ts file) and the config model, going from top to bottom. Underlined are the inputs that Steiger supplies to the queries/commands to the config:

However, you've done some great work here, so ideally we would come to a solution somewhere in the middle. Do you think that what I've described a good idea or would you prefer to keep the code as it is?

[daniilsapa]

I agree with your concern, the config model has become too bloated with responsibilities and code in general. I will try to fix this problem tomorrow, and yes, your diagram offers an interesting idea and will come in handy, thanks!

[daniilsapa]

Hi @illright
I fixed all your minor call outs. Also, I addressed your architectural concern about the config model, hopefully, in a way that you meant

[illright]

Thanks for the changes, it is indeed better. However, I still feel like the solution is more complicated that I'd like it to be, which might impact maintainability in the long run. The concrete points of complexity, in my opinion, are:

1. The apply-severity-globs-to-vfs feature. From the perspective of Steiger's business domain, and to use the FSD understanding of a feature, this isn't a feature because it doesn't do anything meaningful to the user by itself. That's why I find this feature and its abstractions VfsWithSeverity and SeverityMarked* hard to understand.
2. The flat representation of the VFS (record of path to VFS object). The fact that there are several representations for the same thing makes it hard for me to understand what parts of code are compatible with what other parts.
3. Stepping out of Effector philosophy in certain places. I noticed that in some places in code we have .getState() calls and getter functions, and I believe that Effector is meant to work a bit differently, instead with derived stores and meaningful events (for example, "load this config") and effects.
4. Consistency of error production and handling. Currently, errors are thrown in many places, for example, when building a validation schema, when creating rule instructions, when accessing some of the getters, when parsing a config with Zod. I'm getting lost in these places because they also include human-readable error messages, which ideally I'd prefer to keep all in one place, somewhere higher up, like in app.ts, to keep a close look over the amount, variety, and clarity of possible error messages. There are also ways to handle errors in Effector using effects, which might also be a good solution if we isolate this functionality well.

I really wish we had a more real-time communication channel than simply back-and-forth with reviews. I can think of Telegram, Discord, or even maybe a video call to do some brainstorming. What do you think?

[daniilsapa]

@illright

1. I agree that it's not a feature from Steiger's business domain perspective. So, I'll consider moving it to another place.
   I described the need for severity-marked FS entities here:

The function that I wrote for glob matching did not fully suit our needs because it removed nodes from the tree. And as I saw we needed more of a thing that would mark nodes instead of removing them because we’ll need to run globs on a tree several times e.g. for the following config. We need to disable /shared first and then re-enable it (yeah, we discussed that it’s a weird config, but it still could be specified). So, I decided that we needed to mark/remark nodes for every set of globs provided for a rule, and only when all sets are processed we can safely remove nodes that remained “off”

const config = [
    ...fsd.configs.recommended,
    {
      files: ['./src/shared/**'],
      rules: {
        'fsd/public-api': 'off',
      },
    },
    {
      files: ['./src/shared/**'],
      rules: {
        'fsd/public-api': 'error',
      },
    },
],

2. The flat representation of VFS is used only inside apply-severity-globs-to-vfs because it's convenient to present it in that way. But it consumes and produces a tree-shaped VFS, so you don't need to think of different representation outside that one module
3. Got it, I'll be delving deeper into Effector (I just heard about it and had never used it before this project) to make some parts fit better with its philosophy.
4. Agree, I'll refactor it, so the errors are thrown in a more expected manner

And yes, I think it would significantly increase productivity and improve coordination. In fact, I was thinking of suggesting the same. Telegram would be the most convenient option. I will send you an invite on Linkedin and share my contact.

I really wish we had a more real-time communication channel than simply back-and-forth with reviews. I can think of Telegram, Discord, or even maybe a video call to do some brainstorming. What do you think?

[daniilsapa]

And yes, about the visualizer, looks like it could be highly reusable and be useful not just for globs but also in some examples in the docs and learning materials

[daniilsapa]

Hi @illright

I pushed my progress partially, could you take a look at /shared/globs when you have time? It looks like it’s aligned with your idea.




P.S. I know that there are some cross-imports in /shared now. I’ll consider moving some utils inside shared/globs if they are used only there or moving applyExclusions to another layer.

[illright]

Yes, this is quite nice! One minor thing — could you please keep index files as simple re-exports, and move logic in new adjacent files? I usually use index files to quickly get an understanding of what a certain module is capable of, and having logic there gets in the way of that a bit.

[daniilsapa]

Hi @illright

A few notes on my changes:

• Yes, that requirement made perfect sense, so I brought the index file to the format where it just re-exports other files, and did that for several other modules too. (BTW, shouldn't we add such conventions to some document, so the contributors can start delivering code that follows them faster?)
• I got rid of vfs-severity-map completely. Indeed, after I implemented your idea, I could remove a lot of code and now the code base looks more straightforward and clean (also the number of lines was reduced drastically by ~600). I'm happy that you insisted on this idea, it's great 👍.
• I moved several file-system utilities (removeEmptyFolders, copyNode) to /shared/globs module. Because we used them only there, also it looks like applying a glob should be the only way to change an FS tree (I don't see any other scenarios so far).
• After I made my changes I noticed that ramda is used in just 2 places, so I decided to remove it.

[illright]

I'm not done reviewing yet, thought I'd drop a few early comments

[illright]

One little bug to fix

[illright]

Okay, I think this is almost good to go! Could you please run https://knip.dev to get rid of some unused code and exports? The solution itself looks good :)

[illright]

Thank you so much for sticking through to the end! I ran some tests now, I'm quite happy with how it works, let's merge it :)