api icon indicating copy to clipboard operation
api copied to clipboard

Remove cutoff_frequency, deprecated in es7.3.0, forbidden in es8

Open michaelkirk opened this issue 2 years ago • 1 comments

Here's the reason for this change :rocket:

In pursuit of eventually supporting es8, I've dropped some es6 only behavior in https://github.com/pelias/query/pull/134.

It's a draft because it depends on https://github.com/pelias/query/pull/134 being released first.


Here's what actually got changed :clap:

  • integrates the updated version of pelias-query which drops cutoff_frequency.
  • updates the tests.

Here's how others can test the changes :eyes:

Other than npm test, I've run the north-america tests.

Note: The north-america tests aren't all passing, but the ones that are failing are exactly the same as when I run from current master. I'm assuming (🤞) that the failures reflect changes in the input data since the tests were updated, as hypothesized over here.

Aggregate test results
Pass: 233
Improvements: 18
Expected Failures: 28
Placeholders: 0
Regressions: 84
Total tests: 363
Took 7774ms
Test success rate 76.86%

north-america integration test output before north-america test output after

michaelkirk avatar Jul 21 '23 23:07 michaelkirk

re: build failure https://github.com/pelias/api/actions/runs/5627436077

Because of fa9838fed8b7f43603af65d5282561c55a9a44fb, it looks like I need to add my own repo/org variables in order for tests to pass. I've done that here:

Screenshot of org settings, showing setting UBUNTU_VERSION to ubuntu-22.04

...and now tests are passing

(except for the docker --push, for which I haven't configured secrets. It seems like it'd be more convenient to use the pre-existing secrets.GH_TOKEN rather than requiring manual config, but that's a separate issue).

It's not a big deal, but I expect others will get confused by this as well.

michaelkirk avatar Jul 27 '23 01:07 michaelkirk

Hi @michaelkirk, I've merged https://github.com/pelias/query/pull/134, can you please update this PR so the package.json dependency points to the latest pelias/query (which should be 11.2):

npm install pelias-query@latest --save

.. and then open it up for review, tx!

missinglink avatar Apr 18 '24 14:04 missinglink

Done (and rebased).

michaelkirk avatar Apr 18 '24 19:04 michaelkirk

I missed an occurrence somehow. Let me fix this up, retest, and I'll re-open shortly.

michaelkirk avatar Apr 18 '24 19:04 michaelkirk

You didn't miss the macrocounty one, I added that a few weeks ago in https://github.com/pelias/api/pull/1552 🙏

missinglink avatar Apr 19 '24 11:04 missinglink