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

A token value might be a float #124

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Conversation

PowerKiKi
Copy link

@PowerKiKi PowerKiKi commented Jul 11, 2024

@derrabus, this a new version of #119, which might better explain the issue. I'll polish the code if we agree that this might get merged. What do you think ?

Closes #119

@greg0ire
Copy link
Member

greg0ire commented Jul 11, 2024

@PowerKiKi I addressed the CI issues. Now, is this really a bug? Or is it a new feature? Because right now, if you try to implement a lexer such as the one in your test, static analysis points out that you're doing something wrong.

PowerKiKi added a commit to unil-lettres/dilps-tiresias that referenced this pull request Jul 11, 2024
Except:

- doctrine/lexer:^2.1 because of doctrine/lexer#124
@PowerKiKi
Copy link
Author

Thanks you for your help with the CI.

I am coming from https://github.com/creof/doctrine2-spatial, that makes use of https://github.com/creof/wkt-parser, which in turns ended up using doctrine/lexer. My test case is basically a copy-paste of that method. So I am not a direct user of doctrine/lexer, but my project works with doctrine/lexer:^2.1, but crash with doctrine/lexer:^3.0, so from my point of view this is a bug.

But I suppose we could argue that it is a misusage by creof/wkt-parser ? Since I did not write any of those codebase, it's not my place to tell. I suppose it is up to you.

Also, in addition to all of that, the fork longitude-one/doctrine-spatial uses the fork longitude-one/wkt-parser. And while those forks are better maintained, they still use essentially the same code. So they are bound to see that failure too. I am not sure why it didn't happen yet 🤔 ...

I am thinking this comes down to the fact that PHP output arguments are not type checked. AbstractLexer::getType() requires to pass in a string, but it incorrectly assume that will come out as string|int, whereas it actually is mixed, because anything can be affected $value. So I think Token::__construct() must accept mixed, even though it is most often a string... but not always.

So what about we change that to the even more permissive mixed ?

@greg0ire
Copy link
Member

I think we should stick with scalars, mixed feels like opening a can of worms. Let's retarget to 3.1.x and add |bool then?

@PowerKiKi
Copy link
Author

Sure thing, I'll do that. Does the test looks reasonably written as-is ? or should it be refactored differently ?

@PowerKiKi PowerKiKi changed the base branch from 3.0.x to 3.1.x July 12, 2024 07:03
@PowerKiKi PowerKiKi force-pushed the support-float branch 2 times, most recently from 2c7eeda to 8eac7e7 Compare July 12, 2024 07:05
@PowerKiKi
Copy link
Author

The workflow needs to be approved for the CI to run...

@greg0ire
Copy link
Member

It seems there are CI jobs failing. Please take a look at this guide for more on how to handle those.

@greg0ire
Copy link
Member

Sure thing, I'll do that. Does the test looks reasonably written as-is ?

It looks good enough. Should the docs be modified as well to explain why one might want to use float or bool in a parser, if there is a good reason?

@PowerKiKi PowerKiKi force-pushed the support-float branch 2 times, most recently from d9f705e to ba7456e Compare July 12, 2024 07:48
@PowerKiKi
Copy link
Author

I pushed CI fixes, but I am afraid I won't be able to help for the documentation. I couldn't find an obvious place for it, and I don't think I am knowledgeable enough to write documentation for this lib.

@greg0ire
Copy link
Member

Please address the remaining CI issues

@PowerKiKi PowerKiKi force-pushed the support-float branch 2 times, most recently from fcc9006 to 8f141db Compare July 13, 2024 03:50
@PowerKiKi
Copy link
Author

Should be good to merge now

greg0ire
greg0ire previously approved these changes Jul 13, 2024
daviserez added a commit to unil-lettres/dilps-tiresias that referenced this pull request Jul 19, 2024
* Replace ngx-layout by natural-layout #10361

* Restore bugsnag

* Prettier sort css properties #10361

* Use eslint to sort HTML attributes #10394

* Drop debug styles #10394

* Upgrade natural-layout 2.0.0

* Disable passwords Safari "Smart quotes" for all password inputs #10443

If "Smart quotes" is enabled for Safari, that means that password
characters might get replaced by other characters without user noticing.
This means that password is not cross-browser compatible anymore. And a
password created in Safari cannot be used in any other browsers if it
contains characters subjected to "Smart quotes", or the other way around.

By unconditionally disabling spellcheck, we disable "Smart quotes", and
thus make password cross-browsers. However, users that created
their passwords on Safari with "Smart quotes" enabled (in the OS
settings),  will **no longer be able to login** if the password contains
characters such as `'`, `"`, `--` (and others ?).

* Update natural

* Avoid native private members #10451

Because we still support Safari < 16.4, that means that native private
members are downleveled in a way that is less performant at runtime and
slightly bigger at bundle size.

See https://riegler.fr/blog/2024-05-17-private-fields-downleveling

* Re-enable bugsnag

* Update natural for layout fixes #10361

* Update natural for layout fixes #10361

* Use a non-broken natural-layout release #10361

* Show table headers even if no result #10361

There are two reasons for this change. First, and most important, is
that a `mat-table` must never be removed from the DOM or else it will
stop listening to data. That means that if a human searches for
something but finds no result, then they will never be able to see
results anymore, unless if they reload the page.

Second, from an UX perspective, it _might_ be preferable to have a less
surprising by always showing the headers, that never change. And the
only "moving" part is the data itself, not the surrounding structure.

* Don't require CLI confirmation if `/sites/` is a symlink

* Non-interactive CI

* Remove blank lines in `imports`

* Remove blank lines in `import`

* NaturalSelectEnum does not capitalize enum name #10356

While most enum names start with a capital, and should do so, there are
cases where we require to start with a lowercase, such as SI unit `kg`

* Migrate away from modules in tests

Instead, use providers, and avoid `imports` in
`configureTestingModule()` even for the component being tested, because
our code is exclusively standalone.

* Add missing providers in tests

* Be strict about Angular extended diagnostics

There is no reason to tolerate warnings

* @ecodev/natural v59 #9896

This has no impact for Dilps

* Each API field definition is lazy loaded #10525

Previously, we lazy-loaded the whole group of fields of QueryType and
MutationType at once. But each field were not lazy-loaded individually.
So a query with a single field, still loaded the schema for all possible
queries. Same for mutations.

Now we enforce lazy-loaded individual fields via PhpStan extra typing.
The measured speed improvement is actually extremely small, and possibly
even non-existent (!), even for our largest projects. Something like
1.01 ± 0.09 times faster than before.

But we still keep it, because the work is already done, and it
gives other minor advantages, such as:

- less indented code
- easier access to specific mutation via name instead of numerical index
- _maybe_ less memory usage (though I didn't measure it)

* Properly lazy load generated types too #10525

* Happy Stan #10525

* API types Date and Chronos accept native Date as input #10503

Because we implicitly transform them into string via Natural's HTTP
link

* Drop `allowedHosts` because it has no effect anymore #10503

* Linting in pre-commit hook #10483

Not everything can be fixed automatically by eslint, but at least we
avoid pushing broken things to CI.

* Update Felix for fixed native enum in queries #10570

* Update all PHP deps #10257

Except:

- doctrine/lexer:^2.1 because of doctrine/lexer#124

* Use DB strict mode when inserting test data

Because we should know exactly what we are doing and not accidentally
cast things to unexpected values. So we can avoid bugs as seen in commit
894741693987b066f9fe470db3bd9c45a15d3418

* Angular 18.1 and all JS deps #10322

Except:

- @types/node, we run on node 18

* Leverage `provideAnimationsAsync()` #10322

For an initial total ~50KB smaller

* Leverage `eventCoalescing` #10322

For small performance boost when handling events

* Use TypeScript bundler module resolution #10322

The TypeScript language service benefits from this option by allowing
import auto-discovery to leverage any exports defined for a package.
This is particularly useful for packages such as `@angular/material`
which makes extensive use of secondary package exports.

* Fix ngscroll thumb width and color (according to theme)

ngx-scrollbar plugin changed again the name of the property to set the
thumb thickness.

We needed to change the property appareance to compact too to avoid
horizontal scrollbar to appears because of the presence of the vertical
scrollbar.

* Fix multiple vertical scrolls on mass edit modal

And fix app-card right border that should not be displayed on this
modal.

---------

Co-authored-by: Adrien Crivelli <[email protected]>
Co-authored-by: David M <[email protected]>
@PowerKiKi
Copy link
Author

Does this PR needs more than one approval to be merged ? or can it already be merged ?

@greg0ire
Copy link
Member

I'd like another review

psalm.xml Outdated Show resolved Hide resolved
tests/AbstractLexerTest.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

  1. git rebase -i origin/3.1.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

That means that `Lexer\::getType()` might transform `$value` into
string|int|float|bool, but never into objects.
@PowerKiKi
Copy link
Author

Squashed

@greg0ire greg0ire merged commit 042e47e into doctrine:3.1.x Jul 29, 2024
8 checks passed
@greg0ire
Copy link
Member

Thanks @PowerKiKi !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants