I have looked at maybe ~100 errors in the last week as part
of fixing the tooling, and it's quite common to want to just
see what the improved file would look like. Now I show the
desired output with line numbers.
I also try to encourage devs to scroll up, since newbies
often don't do that for some reason when confronted with
error output.
Finally, I add some color. I try to repeat myself without
color for certain things in case colors on certain
backgrounds are hard to read.
A fast way to test this is to just break up a long tag
into two lines.
`Press Enter to send` used to hide `Send` button, we remove that
behaviour.
We show the current state of `Enter` hotkey action via text below
`Send` button which can toggle behaviour on click.
Now we only tokenize the file once, and we pass
**validated** tokens to the pretty printer.
There are a few reasons for this:
* It obviously saves a lot of extra computation
just in terms of tokenization.
* It allows our validator to add fields
to the Token objects that help the pretty
printer.
I also removed/tweaked a lot of legacy tests for
pretty_print.py that were exercising bizarrely
formatted HTML that we now simply ban during the
validation phase.
This accomplishes a few things:
* lighten the load for the main validation loop
* defer indentation checks until we are sure the author
even knows how to match up tags
* add some info to the Token objects that we may soon
consume in our pretty-printer
We now complain about programmers who don't use
4-space indents in template files, rather than
letting the pretty printer fix them.
This is partly just to simplify the pretty printer
code (in future commits), but it also makes the
symptom more obvious to newbie developers. They
are probably just as able to react to the direct
error messages as they are able to figure out how
to read diffs from the pretty printer and grok
the --fix syntax. And once they learn the convention
and configure their editor, it should then be a
one time problem.
We now create tokens for whitespace and text, such that you
could rebuild the template file with "".join(token.s for
token in tokens).
I also fixed a few bugs related to not parsing
whitespace-control tokens.
We no longer ignore template variables, although we could do
a lot better at validating them.
The most immediate use case for the more thorough parser is
to simplify the pretty printer, but it should also make it
less likely for us to skip over new template constructs
(i.e. the tool will fail hard rather than acting strange).
Note that this speeds up the tool by almost 3x, which may be
slightly surprising considering we are building more tokens.
The reason is that we are now munching efficiently through
big chunks of whitespace and text at a time, rather than
checking each individual character to see if it starts one
of the N other token types.
The changes to the pretty_print module here are a bit ugly,
but they should mostly be made irrelevant in subsequent
commits.
String 'Here are a few messages I understand:'(next commit) was failing
./tools/check-capitalization check because of the capital I. I added
'I understand' to the IGNORED_PHRASES list in tools/lib/capitalization.py.
Adding "I" was working as well but didn't seem to me as a very great fix.
Strangely enough, adding " I " to the list made the test fail again
(With a lot of failed strings this time) as mentioned in the following
CZO thread.
Relevent CZO chat -
https://chat.zulip.org/#narrow/stream/49-development-help/topic/capitalization.20confusion.2E
In https://github.com/jorisroovers/gitlint/pull/246 I split the
gitlint package into gitlint and gitlint-core, where the latter avoids
pinning exact versions of its requirements so we can use it again.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
I rewrote most of tools/lib/pretty-printer.py, which
was fairly easy due to being able to crib some
important details from the previous implementation.
The main motivation for the rewrite was that we weren't
handling else/elif blocks correctly, and it was difficult
to modify the previous code. The else/elif shortcomings
were somewhat historical in nature--the original parser
didn't recognize them (since they weren't in any Zulip
templates at the time), and then the pretty printer was
mostly able to hack around that due to the "nudge"
strategy. Eventually the nudge strategy became too
brittle.
The "nudge" strategy was that we would mostly trust
the existing templates, and we would just nudge over
some lines in cases of obviously faulty indentation.
Now we are bit more opinionated and rigorous, and
we basically set the indentation explicitly for any
line that is not in a code/script block. This leads
to this diff touching several templates for mostly
minor fix-ups.
We aren't completely opinionated, as we respect the
author's line wrapping decisions in many cases, and
we also allow authors not to indent blocks within
the template language's block constructs.
In cases where an opening tag is so long that we stretch
it to 2+ lines of code, we should try to use block-style
formatting in the template code.
Unfortunately, we have lots of legacy code that violates
this concept, so this is a timid fix.
There are also legit use cases like textarea where we
probably need to keep the ugly template syntax for things
to render properly.
We disallow this HTML:
junk-text-before-open-tag<p>
This is a paragraph.
</p>
We rarely see the above mistake, but we want to eliminate
the possibility to be somewhat rigorous, and so that we
can eliminate a pretty-printer mis-feature.
Previously, running `./tools/run-dev.py` when provision was required
would lead to a warning along the lines of:
```
Before we run tests, we make sure your provisioning version
is correct by looking at var/provision_version, which is at
version 165.1, and we compare it to the version in source
control (version.py), which is 165.2.
It looks like you checked out a branch that has added
dependencies beyond what you last provisioned. Your command
is likely to fail until you add dependencies by provisioning.
Do this: `./tools/provision`
If you really know what you are doing, use --skip-provision-check to
run anyway.
```
The assumption that we're trying to run tests might cause some
confusion, especially if its the first time you're seeing the
provision warning. Hence, we reword the first paragraph to avoid
making that assumption.
The second paragraph has also been slightly altered, since (1) it's
possible that we didn't checkout a different branch, but eg just
rebased with upstream and (2) we might not be on a VM.
The warning you'd get after this commit would be along the lines of:
```
Provisioning state check failed! This check compares
`var/provision_version` (currently 165.2) to the version in
source control (`version.py`), which is 164.6, to see if you
likely need to provision before this command can run
properly.
The branch you are currently on expects an older version of
dependencies than the version you provisioned last. This may
be ok, but it's likely that you either want to rebase your
branch on top of upstream/main or re-provision your machine.
Do this: `./tools/provision`
If you really know what you are doing, use --skip-provision-check to
run anyway.
```
or along the lines of:
```
Provisioning state check failed! This check compares
`var/provision_version` (currently 165.2) to the version in
source control (`version.py`), which is 167.2, to see if you
likely need to provision before this command can run
properly.
The branch you are currently on has added dependencies beyond
what you last provisioned. Your command is likely to fail
until you add dependencies by provisioning.
Do this: `./tools/provision`
If you really know what you are doing, use --skip-provision-check to
run anyway.
```
This will be useful to let users enable/disable
sharing read receipts once we add that feature.
Note: Added "I've" to IGNORED_PHRASES in
tools/lib/capitalization.py to avoid capitalization
errors for the label text of this setting.
gitlint has a bunch of pinned requirements that hold back important
upgrades and conflict with other packages’ requirements. The gitlint
author has rejected proposals to unpin them because it might increase
the amount of maintenance he needs to do
(https://github.com/jorisroovers/gitlint/pull/133). That decision is
his to make, but _somebody_ needs to do the maintenance, so we
delegate it to Debian and Ubuntu. If that means using a significantly
older version of gitlint, that’s a tradeoff we need to make to keep
the rest of our requirements current.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
These changes are all independent of each other; I just didn’t feel
like making dozens of commits for them.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We recently added a lot of new pages to our top navigation and
restructured top-navigation in general. This commit updates the
footer to reflect the recent changes to our top navigation.
We currently have created a copy of the
`clean_unused_caches.main` function in
`provision_inner.py` to clean the unused caches. But as
we have now converted the script into a python file we
can directly call that function.
This commit replaces that function (introduced in adc0ed4206) with
`clean_unused_caches.main`.
Thumbor and tc-aws have been dragging their feet on Python 3 support
for years, and even the alphas and unofficial forks we’ve been running
don’t seem to be maintained anymore. Depending on these projects is
no longer viable for us.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This reverses the policy that was set, but incompletely enforced, by
commit 951514dd7d. The self-closing tag
syntax is clearer, more consistent, simpler to parse, compatible with
XML, preferred by Prettier, and (most importantly now) required by
FormatJS.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We use GIPHY web SDK to create popover containing GIFs in a
grid format. Simply clicking on the GIFs will insert the GIF in the compose
box.
We add GIPHY logo to compose box action icons which opens the GIPHY
picker popover containing GIFs with "Powered by GIPHY"
attribution.
We should still display the `source` advice when not in Vagrant or a
Droplet, since that's an important hint for anyone using local
installation on Linux.
We move the "If you are using Vagrant..." text a bit after to
highlight things nicely for folks who are running tools outside
Vagrant.
Also tighten text to avoid line-wrapping on an 80 character console.
This also fixes the suggestions for the following words: disabled,
disables, disabling, implemented, implementing, implements, kept,
made, took, using.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We have disabled CircleCI and are using GitHub Actions for automated
testing.
docs: Changed context from CircleCI to Github Actions and wrote
some documentation specific to GH Actions.
tools: Replaced env checks for CIRCLECI with GITHUB_ACTION.
README: Use GitHub Actions build status badge.
GitHub Actions supports doing more than just CI,
and so in some contexts it's less obvious that we're
talking about just the CI if we refer to it instead of CircleCI.
When exception is raised inside an exception handler, Python 3
helpfully prints both tracebacks separated by “During handling of the
above exception, another exception occurred:”. But when we’re using
an exception handler to retry the same operation, multiple tracebacks
are just noise. Suppress the earlier one using PEP 409 syntax.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This commit changes help message for skip-provision-check argument
used with various test commands. The message is changed with an
intention to be more clear about what this option actually does.
This commit is in series of various changes done to provide clarity
over the use of --force option which is renamed to
--skip-provision-check.
Fixes: #17455
This commit renames --force argument used with various tests to
--skip-provision-check. As a consequence of this name change all other
files that set --force option for the test commands have been updated.
This change is done in order to provide more clarity for using this
option for runnning tests.
This commit addresses issue #17455.
This commit moves --force option used with various tests to
test-scripts.py to have it alongside the logic that does provisioning
status assertion.
This is a step towards providing more clarity over use of this
argument with tests as asked in issue #17455.
The "Narrow to PM with" notification above the composebox was
double-escaped, mangling names with single quotes in them. This removes
the escaping in i18next, causing the name to be escaped only in
handlebars.
Replaced methods/functions of moment.js with date-fns library.
The motive was to replace it with a smaller frontend timezone library.
Date-fns ~ 11.51 kb
moment.js ~ 217.87 kb
Some of the format strings change because date-fns encodes them
differently from how moment did.
Fixes#16373.
The node package allow use to control xvfb apt package in puppeteer
tests. This help us create a fake display so we can run puppeteer in
headful (headless: false) mode, which is required to use the chrome
extension desktop capture API.
Doing service memcached start instead of restart fixed an issue on
focal build in GitHub actions, where it exits with code 1 when it
is done twice.It is done first in Install Dependencies step and then
again in last step where we call tools/ci/setup-backend again which
runs provision.
Furthermore, I don't belive there is a technical reason we use
restart over start; rather I think it was just a random choice with
the intend to just start the services in CI. I traced the code back
to commit 1f2f497cab if it helps.
Looking at the source code of memcached, the step that's failing is:
start-stop-daemon --stop --quiet --oknodo --pidfile $PIDFILE
which is equivilent to: service memcached stop, we can rule out the
service memcache start since it works. Ideally, we do figure out and
solve the issue of why memcached fails when executing service
memcached stop but I am not equipped with debugging it. And this
workaround seems reasonable rather than a "hacky" solution.
For the relevant code in memcached see:
https://github.com/memcached/memcached/blob/master/scripts/memcached-init.
Finally, the change to the rest of services is for consistency.
The automated tests running in CircleCI don't actually use the `zulip`
db, so we can skip running migrations on it in some CircleCI shards to
save time.
NOTE: This only effects build jobs that run provision, except the
`production-build` job where we skip building the dbs altogether.
Migrations still run on `focal-backend` build job to ensure
we are testing all our development setup code.
These files can’t use f-strings yet because they need to run in Python
2 or Python 3.5.
Generated by pyupgrade.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Generated by pyupgrade --py36-plus --keep-percent-format.
Now including %d, %i, %u, and multi-line strings.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Use read-only types (List ↦ Sequence, Dict ↦ Mapping, Set ↦
AbstractSet) to guard against accidental mutation of the default
value.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Fixes#2665.
Regenerated by tabbott with `lint --fix` after a rebase and change in
parameters.
Note from tabbott: In a few cases, this converts technical debt in the
form of unsorted imports into different technical debt in the form of
our largest files having very long, ugly import sequences at the
start. I expect this change will increase pressure for us to split
those files, which isn't a bad thing.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Automatically generated by the following script, based on the output
of lint with flake8-comma:
import re
import sys
last_filename = None
last_row = None
lines = []
for msg in sys.stdin:
m = re.match(
r"\x1b\[35mflake8 \|\x1b\[0m \x1b\[1;31m(.+):(\d+):(\d+): (\w+)", msg
)
if m:
filename, row_str, col_str, err = m.groups()
row, col = int(row_str), int(col_str)
if filename == last_filename:
assert last_row != row
else:
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
with open(filename) as f:
lines = f.readlines()
last_filename = filename
last_row = row
line = lines[row - 1]
if err in ["C812", "C815"]:
lines[row - 1] = line[: col - 1] + "," + line[col - 1 :]
elif err in ["C819"]:
assert line[col - 2] == ","
lines[row - 1] = line[: col - 2] + line[col - 1 :].lstrip(" ")
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Generated by pyupgrade --py36-plus --keep-percent-format, but with the
NamedTuple changes reverted (see commit
ba7906a3c6, #15132).
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Since we use this option in our docker-zulip project also
so rather than using it as a test suite option we made it
more specific i.e. --build-release-tarball-only.
Some UTF-8 characters (★ for example) are not displayed correctly, with
fonts-liberation. Puppeteer recommends[1] installing fonts-freefont-ttf in
their docs on running Puppeteer in docker.
Provisioning forward is sufficient. There's no need to remove the
new font and replace it with the old font, I think.
[1]: https://github.com/puppeteer/puppeteer/blob/master/docs/troubleshooting.md#running-puppeteer-in-docker
We now use the `--streamlined` options for `run-dev.py`
when we use `test_server_running` for `test-api` and
`test-js-with-casper` (and its experimental
replacement, `test-js-with-puppeteer`).
This means we don't slow anything down with
processes like thumbor, process_fts_updates, etc.,
which aren't meaningfully exercised by these tests.
We may eventually want some tests to meaningfully
exercise those processes, and when that day comes,
we will need to add an extra argument to
`test_server_running`, probably, but until then,
we just always set `--streamlined` in that codepath.
There is actually a tool called `./tools/test-run-dev`
that we run in CI, and it will use the full mode.
It just doesn't verify much stuff--it mostly polls
the server without testing specific features.
This seems to save about 1s of the startup time on a system I use
(~10.6s -> ~9.7s).
This will give help up write new digest only if the db rebuild
succeeds. We were relying on the caller to
be successful in building db, this was hacky and unreliable.
We write new db digest once the caller succeeds, this ensures
that we write new digest after every successful attempt.
This fixes the anomality we were facing that Databases were rebuild
on the 2nd provision attempt with no changes to files or migrations.
This was happening because we didn't write a new digest for db
after the first provision (The case of DB didn't exist).
During the 1st provision, we check the template_status() of
Database both Dev and Test, but database_exists() of Databases
obviously returned false, and we rebuild the database,
but forgot to write_new_digest and hence the anomaly in the
second provision explained above.
Yes, it's slightly janky to create an
argparse.Namespace object like this, but it
saves us from shelling out to a script whose
only real value-add is parsing a single
`threshold_days` argument.
This saves about 130ms for a no-op provision.
We now prevent these variations:
* <hr/>
* <hr />
* <br/>
* <br />
We could enforce similar consistency for other void
tags, if we wished, but these two are particularly
prevalent.
Since in travis we don't have root access so we used to add different
srv path. As now we shifted our production suites to Circle CI
we don't need that code so removed it.
Also we used a hacky code in commit-lint-message for travis which is
now of no use.
We now forbid tags of the form `<foo ... />` in most
places, and we also forbid it even for several void
tags.
We make exceptions for tags that are already formatted
in two different ways in our codebase. This is mostly
svg tags, plus these common cases:
- br
- hr
- img
- input
It would be nice to lock down a convention for these,
even though the HTML specification is unopinionated
on these. We'll probably want to stay flexible for
svg tags, since they are sometimes copy/pasted from
other sources (although it's probably rare enough for
them that we can tolerate just doing minor edits as
needed).
If folks put something like '<br/>' in the HTML,
we would think the tag's name was "br/" instead
of "br". I think we were assuming most folks
would write either "<br>" or <br />".
ASIDE:
We should probably have a consistent
preference among these styles:
* <br>
* <br/>
* <br />
I prefer the first.
Steve Howell
Aman Agrawal
Palash
Anders Kaseorg
Sahil Batra
YashRE42
Alex Vandiver
Kevin Scott
Dinesh
Eeshan Garg
manavdesai27
Riken Shah
Gilbert Bishop-White
Gaurav Pandey
Tim Abbott
Ganesh Pawar
m-e-l-u-h-a-n
Wesley Aptekar-Cassels
aryanshridhar
aditya-mitra
Sutou Kouhei
Priyank Patel
Vishnu KS
Felix
Anders Kaseorg
arpit551
Puneeth Chaganti
Steve Howell
Aman Agrawal
Tim Abbott