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

Add formatter option to the VSCode extension #98

Open
ameshkov opened this issue Jul 3, 2023 · 10 comments
Open

Add formatter option to the VSCode extension #98

ameshkov opened this issue Jul 3, 2023 · 10 comments
Labels
feature request Request to introduce a new feature question Further information is requested

Comments

@ameshkov
Copy link
Member

ameshkov commented Jul 3, 2023

At this point this is an issue for discussing how the formatter can be implemented and how exactly the filter lists and individual rules should be formatted / sorted.

My initial idea is to sort the filter lists so that all rules for a given website were collected together. Something like this:

google.com###banner
||example.org^$domain=google.com
||google.com/test

At the same time we should take comments into account. Maybe allow grouping rules into a single section using special comments.

Anyways, lets discuss.

CC @AdguardTeam/filters-maintainers @maximtop @scripthunter7

@ameshkov ameshkov added question Further information is requested feature request Request to introduce a new feature labels Jul 3, 2023
@scripthunter7
Copy link
Member

scripthunter7 commented Jul 3, 2023

@ameshkov

At this point this is an issue for discussing how the formatter can be implemented

I see two main ways of implementation:

A while ago I collected a basic concept for the rule orderer. You can find it here: https://github.com/scripthunter7/adblock-rule-orderer

I would separate the following steps:

  • grouping of rules (by category, domain or anything else)
  • sorting of rules within a group
  • way of formatting rules (for example, put space before !important in CSS injections, single quoted or double quoted scriptlet args, etc)

My initial idea is to sort the filter lists so that all rules for a given website were collected together

I also thought about grouping the rules by websites, but what should happen if a filtering rule contains several different domains? For example:

example.com,example.net###banner
/script-to-block.js^$script,domain=example.com|example.org|~example.net

or if one rule belongs to the scope of a preprocessor directive, while the others don't:

example.com###banner-1
example.com###banner-2

!#if (adguard_ext_safari || adguard_app_ios || adguard_ext_android_cb)
@@/script-to-block^$script,domain=example.com
! ...
! other rules
!#endif

Should these rules be moved to some kind of mixed category?

@scripthunter7
Copy link
Member

We also should consider to keep the formatting in sync with the planned code guidelines: AdguardTeam/CodeGuidelines#16

@maximtop
Copy link
Contributor

maximtop commented Jul 4, 2023

Not clear how to resolve cases when one comment is related to a group of rules

@ameshkov
Copy link
Member Author

ameshkov commented Jul 4, 2023

@maximtop we can provide special rules for that, something like this:

! block start

! block end

@scripthunter7
Copy link
Member

If we introduce such rules, perhaps it would make sense to follow the pattern of preprocessor directives:

!#section
rules
!#endsection

@ameshkov
Copy link
Member Author

ameshkov commented Jul 5, 2023

IMO, formatter and linter are different things and mixing them together may be confusing.

At the same time, it does not mean that AGLint cannot enforce formatting rules, it can rely on the formatter for that.

@scripthunter7
Copy link
Member

scripthunter7 commented Jul 12, 2023

After all, AGTree is the basis, linter and formatter can also be built on it.

It is important to divide the formatter into two categories:

  • formatter
    • just formatting rules (but not sorting them), eg format
      #$#body { padding : 2px !important;}
      to
      #$#body { padding: 2px !important; }
  • sorter / orderer
    • group & sort rules within a file

Separate config schemas should be prepared for both categories.

@Alex-302
Copy link
Member

My initial idea is to sort the filter lists so that all rules for a given website were collected together.

We use this approach for some sites. In general it is not a good idea to apply it for all rules, because some rules can be applied to many sites, they can have mirrors (not only with different TLD), and they merged into one large rule.

At the same time we should take comments into account. Maybe allow grouping rules into a single section using special comments.

In some cases we are using Comment Anchors to improve file navigation

! SECTION: Cookies - Popular rules
||rule1.com^
||ruleN.com^
! NOTE: Popular rules end ⬆️
! !SECTION: Cookies - Popular rules
How it looks:

image

Automatically creating groups will not improve convenience.
As written above - there may (will) be problems with existing groups, directives and hints.
Previously never felt the need to create a separate group for each site. This may be handy for the hotfixes filter, but not for the main filters.

@ameshkov
Copy link
Member Author

The problem with the current structure in AdguardFilters is that it's hard to understand where to add a new rule unless you already have lots of experience working with it. I don't think we should take it as an example. Instead of that, let's think what structure would be reasonable to use if we were creating a new list from scratch.

@scripthunter7
Copy link
Member

IMO, formatter and linter are different things and mixing them together may be confusing.

At the same time, it does not mean that AGLint cannot enforce formatting rules, it can rely on the formatter for that.

@ameshkov ESLint also drops formatting rules: https://eslint.org/blog/2023/10/deprecating-formatting-rules/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request to introduce a new feature question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants