mirror of
https://github.com/zulip/zulip.git
synced 2025-11-03 05:23:35 +00:00
tools: Remove find-add-class tool.
I added this tool a few years ago, and I did have a vision for how it would improve our codebase, but I can't remember exactly where I was going with it. At this point the tool is just a little too noisy to be helpful. An example of it creating confusion was a recent PR where somebody was patching user_circle_class in the PM list, and we already had similar code in the buddy list, because they use the same CSS. I mean, there was possibly a way that the code could have been structured to remove some of the duplication, but it probably would have just moved the complexity around. I just don't think it's worth maintaining the tool at this point.
This commit is contained in:
Steve Howell
Tim Abbott
@@ -185,14 +185,6 @@ that we exempt may be deemed not worthwhile to fix.
|
||||
We check our JavaScript code in a few different ways:
|
||||
- We run eslint.
|
||||
- We perform custom Zulip regex checks on the code.
|
||||
- We verify that all addClass calls, with a few exceptions, explicitly
|
||||
contain a CSS class.
|
||||
|
||||
The last check happens via a call to `./tools/find-add-class`. This
|
||||
particular check is a work in progress, as we are trying to evolve a
|
||||
more rigorous system for weeding out legacy CSS styles, and the ability
|
||||
to quickly introspect our JS code for `addClass` calls is part of our
|
||||
vision.
|
||||
|
||||
#### Puppet manifests
|
||||
|
||||
|
||||
@@ -1,43 +0,0 @@
|
||||
#!/usr/bin/env python3
|
||||
|
||||
from lib.find_add_class import display, find
|
||||
import glob
|
||||
import argparse
|
||||
|
||||
# check for the venv
|
||||
from lib import sanity_check
|
||||
sanity_check.check_venv(__file__)
|
||||
|
||||
def process_files():
|
||||
# type: () -> None
|
||||
|
||||
description = '''
|
||||
Use this tool to find HTML classes that we use in our JS code.
|
||||
This looks for calls to addClass, and if you use the -v option,
|
||||
you will get a display of (fn, html_class) tuples that
|
||||
represent addClass calls.
|
||||
|
||||
If you call it with no options, the tool acts as a linter, and
|
||||
it will complain if it can't resolve the class for an addClass()
|
||||
call.
|
||||
'''
|
||||
|
||||
parser = argparse.ArgumentParser(description=description)
|
||||
parser.add_argument('-v', '--verbose',
|
||||
action='store_true', default=False,
|
||||
help='show where calls are')
|
||||
parser.add_argument('targets', nargs=argparse.REMAINDER)
|
||||
args = parser.parse_args()
|
||||
|
||||
if args.targets == []:
|
||||
fns = glob.glob('static/js/*.js')
|
||||
else:
|
||||
fns = args.targets
|
||||
|
||||
if args.verbose:
|
||||
display(fns)
|
||||
else:
|
||||
find(fns)
|
||||
|
||||
if __name__ == '__main__':
|
||||
process_files()
|
||||
@@ -1,116 +0,0 @@
|
||||
from typing import List, Set, Tuple
|
||||
|
||||
import os
|
||||
import re
|
||||
|
||||
GENERIC_KEYWORDS = [
|
||||
'active',
|
||||
'alert',
|
||||
'danger',
|
||||
'condensed',
|
||||
'disabled',
|
||||
'enabled',
|
||||
'error',
|
||||
'expanded',
|
||||
'fade-out',
|
||||
'first',
|
||||
'hide',
|
||||
'in',
|
||||
'show',
|
||||
'notdisplayed',
|
||||
'popover',
|
||||
'no-border',
|
||||
'rtl',
|
||||
'second',
|
||||
'selected',
|
||||
'slide-left',
|
||||
'success',
|
||||
'text-error',
|
||||
'warning',
|
||||
'zoom-in', # TODO: clean these up, they are confusing
|
||||
'zoom-out',
|
||||
]
|
||||
|
||||
def raise_error(fn, i, line):
|
||||
# type: (str, int, str) -> None
|
||||
error = '''
|
||||
In %s line %d there is the following line of code:
|
||||
|
||||
%s
|
||||
|
||||
Our tools want to be able to identify which modules
|
||||
add which HTML/CSS classes, and we need two things to
|
||||
happen:
|
||||
|
||||
- The code must explicitly name the class.
|
||||
- Only one module can refer to that class (unless
|
||||
it is something generic like an alert class).
|
||||
|
||||
If you get this error, you can usually address it by
|
||||
refactoring your code to be more explicit, or you can
|
||||
move the common code that sets the class to a library
|
||||
module. If neither of those applies, you need to
|
||||
modify %s
|
||||
''' % (fn, i, line, __file__)
|
||||
raise Exception(error)
|
||||
|
||||
def generic(html_class):
|
||||
# type: (str) -> bool
|
||||
for kw in GENERIC_KEYWORDS:
|
||||
if kw in html_class:
|
||||
return True
|
||||
return False
|
||||
|
||||
def display(fns):
|
||||
# type: (List[str]) -> None
|
||||
for tup in find(fns):
|
||||
# this format is for code generation purposes
|
||||
print(' ' * 8 + repr(tup) + ',')
|
||||
|
||||
def find(fns):
|
||||
# type: (List[str]) -> List[Tuple[str, str]]
|
||||
encountered = set() # type: Set[str]
|
||||
tups = [] # type: List[Tuple[str, str]]
|
||||
for full_fn in fns:
|
||||
# Don't check frontend tests, since they may do all sorts of
|
||||
# extra hackery that isn't of interest to us.
|
||||
if full_fn.startswith("frontend_tests"):
|
||||
continue
|
||||
lines = list(open(full_fn))
|
||||
fn = os.path.basename(full_fn)
|
||||
module_classes = set() # type: Set[str]
|
||||
for i, line in enumerate(lines):
|
||||
if 'addClass' in line:
|
||||
html_classes = [] # type: List[str]
|
||||
m = re.search(r'''addClass\(['"](.*?)['"]''', line)
|
||||
if m:
|
||||
html_classes = [m.group(1)]
|
||||
if not html_classes:
|
||||
if 'bar-success' in line:
|
||||
html_classes = ['bar-success', 'bar-danger']
|
||||
elif fn == 'hotspots.js' and 'arrow_placement' in line:
|
||||
html_classes = ['arrow-top', 'arrow-left', 'arrow-bottom', 'arrow-right']
|
||||
elif 'color_class' in line:
|
||||
continue
|
||||
elif 'stream_dark' in line:
|
||||
continue
|
||||
elif 'opts.' in line:
|
||||
continue
|
||||
elif fn == 'signup.js' and 'class_to_add' in line:
|
||||
html_classes = ['error', 'success']
|
||||
elif fn == 'ui_report.js' and 'status_classes' in line:
|
||||
html_classes = ['alert']
|
||||
|
||||
if not html_classes:
|
||||
raise_error(full_fn, i, line)
|
||||
for html_class in html_classes:
|
||||
if generic(html_class):
|
||||
continue
|
||||
if html_class in module_classes:
|
||||
continue
|
||||
if html_class in encountered:
|
||||
raise_error(full_fn, i, line)
|
||||
tups.append((fn, html_class))
|
||||
module_classes.add(html_class)
|
||||
encountered.add(html_class)
|
||||
return tups
|
||||
@@ -51,8 +51,6 @@ def run():
|
||||
'frontend': ['js', 'ts', 'css', 'scss', 'hbs', 'html', 'lock'],
|
||||
}, exclude=EXCLUDED_FILES)
|
||||
|
||||
linter_config.external_linter('add_class', ['tools/find-add-class'], ['js'],
|
||||
description="Compares addClass() between JavaSsript and CSS.")
|
||||
linter_config.external_linter('css', ['node', 'node_modules/.bin/stylelint'], ['css', 'scss'],
|
||||
fix_arg='--fix',
|
||||
description="Standard CSS style and formatting linter "
|
||||
|
||||
Reference in New Issue
Block a user