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

New config structure docs #96

Merged
merged 25 commits into from
Oct 25, 2024

Conversation

daniilsapa
Copy link
Collaborator

Resolves #89

Hi @illright

I added several new sections to README to describe the new structure, its concepts, and how to work with it. Also, I added a migration guide in a separate file, as you requested.

Let me know if you have any suggestions/corrections. Thank you in advance!

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the detailed write-up!

One major concern I have here is the size of the README. Ideally, I'd like to keep the README short, and have a separate resource for plugin authors and people who want to quickly write a rule or two. In the near future, it will probably amount to a documentation site. That's why I don't think we should go into details about concepts like GlobArray, Config Object, etc. These are implementation details and terms rather than public facing components, in my opinion.

So maybe we can just keep the README to one example that includes fsd.configs.recommended, a global ignore, and then a change of severity for a certain glob?

As for the docs about writing plugins, I think we should hold on with this for now. Since we still have some technical and design issues to iron out (mostly on my side, with the toolkit stuff), I don't think we're ready to write docs just yet.

MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
@daniilsapa
Copy link
Collaborator Author

I fixed all the issues. I decided not to delete the examples completely and moved them to a separate page.

@daniilsapa daniilsapa requested a review from illright September 3, 2024 21:03
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@illright
Copy link
Member

illright commented Sep 3, 2024

Sorry for not finding these comments before, the fixes for the previous comments are good, thanks!

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, when reading the README in rendered Markdown, I noticed something else. One of them is a thing that is straight-up no longer true, others are simply clarifications

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
daniilsapa and others added 3 commits September 5, 2024 23:11
Co-authored-by: Lev Chelyadinov <[email protected]>
Co-authored-by: Lev Chelyadinov <[email protected]>
Co-authored-by: Lev Chelyadinov <[email protected]>
@daniilsapa
Copy link
Collaborator Author

Yes, your suggestions were completely reasonable, so I committed them

@daniilsapa
Copy link
Collaborator Author

Also, I thought about merging this PR last, because:

  • While we are buttoning up the new config there can be slight changes to the documentation (e.g. what we decided about GlobalIgnores today)
  • Let's not confuse people with documentation for features that haven't been released yet.

@illright
Copy link
Member

illright commented Sep 5, 2024

Makes sense. Let's wait with this then

@daniilsapa
Copy link
Collaborator Author

@illright
FYI I'm going to update this PR with some information on how our glob matching logic works. Let me know if we need to add anything else.

@daniilsapa daniilsapa requested a review from illright October 5, 2024 11:51
Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Phew, sorry for this big load of corrections, it's just that documentation is something that I am particularly passionate and opinionated about. Let me know if you disagree with anything

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 84 to 94
### Glob matching

All globs are matched only against files, folder severities are computed based on the files inside them. The formula is simple: the folder severity is the highest severity of files inside it (from highest to lowest: error, warn, off).

**Glob examples**:

- `./src/shared/**` - matches all files in the `shared` folder and its subfolders
- `./src/shared/*` - matches all files that are direct children of the `shared` folder
- `./src/shared` - based on the fact that globs are matched against files, this one matches only `shared` file (without an extension) inside the `src` folder
- `**/__mocks__/**` - matches all files in all `__mocks__` folders throughout the project
- `**/*.{test,spec}.{ts,tsx}` - matches all test files in the project
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: again, I believe this is a more technical description, and I forecast that most people will just disable entire folders, so people are unlikely to run up against a case where one file made the whole folder be error.

suggestion: let's just get rid of this entire section

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to move it to CONFIG_EXAMPLES.md so far. Let me know if you think it should not be even there

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally think it shouldn't be even there, but I also don't mind it being there. As long as it's not up front in the README, I'm fine with it

EXAMPLES.md Outdated Show resolved Hide resolved
EXAMPLES.md Outdated Show resolved Hide resolved
EXAMPLES.md Outdated Show resolved Hide resolved
EXAMPLES.md Outdated Show resolved Hide resolved
MIGRATION_GUIDE.md Outdated Show resolved Hide resolved
@daniilsapa
Copy link
Collaborator Author

You don't need to be sorry for these corrections. They are completely reasonable and improve the documentation. Thanks for reviewing!

@illright
Copy link
Member

There's still some instances of import defineConfig. Could you search the project for this and replace it with import { defineConfig }?

Copy link
Member

@illright illright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I took the liberty to introduce some minor corrections to the examples myself, because I'm eager to merge this :) Let me know if they look alright to you, and then we can merge

@daniilsapa
Copy link
Collaborator Author

Thanks for catching that! Yes, I forgot to update some examples based on the new glob-matching approach

@illright illright merged commit 335be11 into feature-sliced:master Oct 25, 2024
7 checks passed
illright added a commit that referenced this pull request Nov 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add information about the new config structure to README.md
2 participants