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.

Sign up for GitHub

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

Jump to bottom

editoast: adapt signal search to use logical_signals and drop aspects #5260

Merged
merged 3 commits into from
Oct 24, 2023
Merged

editoast: adapt signal search to use logical_signals and drop aspects #5260

merged 3 commits into from
Oct 24, 2023

Conversation

leovalais
Copy link
Contributor

@leovalais leovalais commented Oct 9, 2023 •
edited
Loading

Closes #5217

To merge after #5166

@flomonster I've added a new command editoast search refresh since your last review.

@leovalais leovalais requested review from a team as code owners October 9, 2023 16:06
@codecov
Copy link

codecov bot commented Oct 9, 2023 •
edited
Loading

Codecov Report

Merging #5260 (a1dbbdb) into dev (0731aaf) will decrease coverage by 0.11%.
Report is 17 commits behind head on dev.
The diff coverage is 1.70%.

@@             Coverage Diff              @@
##                dev    #5260      +/-   ##
============================================
- Coverage     18.84%   18.73%   -0.11%     
  Complexity     2315     2315              
============================================
  Files           851      852       +1     
  Lines        102677   102948     +271     
  Branches       2403     2403              
============================================
- Hits          19347    19285      -62     
- Misses        81989    82322     +333     
  Partials       1341     1341
Flag Coverage Δ
editoast 67.26% <0.00%> (-2.32%) ⬇️
front 7.11% <4.72%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
editoast/editoast_derive/src/lib.rs 0.00% <ø> (ø)
editoast/src/client/mod.rs 57.14% <ø> (ø)
front/src/common/Map/const.ts 100.00% <ø> (ø)
front/src/common/api/osrdEditoastApi.ts 91.76% <100.00%> (-0.19%) ⬇️
editoast/src/views/search/mod.rs 0.00% <0.00%> (ø)
editoast/src/tables.rs 42.62% <0.00%> (-0.12%) ⬇️
...nt/src/common/BootstrapSNCF/SelectImprovedSNCF.tsx 0.00% <0.00%> (ø)
front/src/common/Map/Search/SignalCard.tsx 0.00% <0.00%> (ø)
front/src/common/BootstrapSNCF/MultiSelectSNCF.tsx 0.00% <0.00%> (ø)
editoast/src/main.rs 15.00% <0.00%> (-4.28%) ⬇️
... and 3 more

... and 5 files with indirect coverage changes

@flomonster
Copy link
Contributor

flomonster commented Oct 10, 2023 •
edited
Loading

A few things to fix

  1. We should be able to search for any type of signals
  2. The naming could be improved (and translated)
  3. The sprite isn't shown

image

flomonster
flomonster requested changes Oct 10, 2023
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR.
The back looks good to me.
Some front stuff has to be fixed to be merged.

@leovalais leovalais force-pushed the lva/signal-search-rework branch 2 times, most recently from af8c4a2 to fe56d2b Compare October 18, 2023 21:54
@leovalais leovalais requested a review from a team as a code owner October 18, 2023 21:54
@leovalais leovalais self-assigned this Oct 18, 2023
@leovalais leovalais added the area:editoast Work on Editoast Service label Oct 18, 2023
@leovalais leovalais requested a review from flomonster October 18, 2023 23:01
@leovalais leovalais requested a review from eckter as a code owner October 19, 2023 08:16
@leovalais leovalais force-pushed the lva/signal-search-rework branch from 526f9b8 to 2d79fe4 Compare October 19, 2023 08:18
@leovalais leovalais removed the request for review from eckter October 19, 2023 08:18
@leovalais leovalais force-pushed the lva/signal-search-rework branch from 2d79fe4 to 48a7b9a Compare October 19, 2023 08:20
flomonster
flomonster reviewed Oct 19, 2023
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Waiting for the merge of #5166 to test it.

@leovalais
editoast: add search migration generation CLI command
2a5f0b7 
Updates the `Search` derive macro to accept a bunch of migration generation settings for each search object.

Adds `search list` and `search make-migration <object> <migration_dir>` commands.
@leovalais
editoast: update search_signal
c4cc260 
Uses standard logical_signals instead of relying on extensions.
@leovalais leovalais force-pushed the lva/signal-search-rework branch from 48a7b9a to adff5ed Compare October 20, 2023 13:07
@leovalais leovalais requested review from flomonster and removed request for a team October 20, 2023 13:10
flomonster
flomonster approved these changes Oct 20, 2023
View reviewed changes
Copy link
Contributor

@flomonster flomonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for this PR!
Reviewed + Tested

clarani
clarani reviewed Oct 23, 2023
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, thanks for your PR 🙏

@leovalais leovalais force-pushed the lva/signal-search-rework branch from adff5ed to a1dbbdb Compare October 24, 2023 12:51
@leovalais leovalais requested a review from clarani October 24, 2023 12:57
clarani
clarani approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@clarani clarani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, tested ✔️

@leovalais leovalais added this pull request to the merge queue Oct 24, 2023
Merged via the queue into dev with commit cf6ebac Oct 24, 2023
@leovalais leovalais deleted the lva/signal-search-rework branch October 24, 2023 14:38
@leovalais leovalais mentioned this pull request Oct 24, 2023
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flomonster flomonster flomonster approved these changes

@clarani clarani clarani approved these changes

Assignees

@leovalais leovalais

Labels
area:editoast Work on Editoast Service
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

adapt the new sign search engine
3 participants
@leovalais @flomonster @clarani