pelias / api

HTTP API for Pelias Geocoder
http://pelias.io
MIT License
220 stars 163 forks source link

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

Closed michaelkirk closed 7 months ago

michaelkirk commented 1 year ago

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:


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 commented 1 year ago

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.

missinglink commented 7 months ago

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!

michaelkirk commented 7 months ago

Done (and rebased).

michaelkirk commented 7 months ago

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

missinglink commented 7 months ago

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