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

Gestion des mots contractés #72

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Gestion des mots contractés #72

wants to merge 4 commits into from

Conversation

tmerlier
Copy link
Contributor

@tmerlier tmerlier commented Feb 1, 2017

Gestion des mots contractés:

  • st => saint
  • ste => sainte
  • cgne => campagne

fix #61 Mots contractés

@tmerlier tmerlier added this to the Rentrée des classes milestone Feb 1, 2017
@tmerlier tmerlier requested a review from jdesboeufs February 1, 2017 15:59
lib/communes.js Outdated

const schema = {
nom: {
type: 'text',
queryWith: 'nom',
ref: 'code',
replacePatern: abbreviations,
Copy link
Contributor

Choose a reason for hiding this comment

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

replacePatterns

@@ -1,4 +1,5 @@
const normalizeString = require('../normalizeString');
const replaceAbbreviations = require('../replaceAbbreviations');
Copy link
Contributor

Choose a reason for hiding this comment

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

Le nom me parait trop spécifique

Copy link
Contributor

Choose a reason for hiding this comment

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

À modifier

@@ -7,6 +8,7 @@ class TextIndex {
if (!key) throw new Error('key is required');
this._key = key;
this._boost = options.boost || {};
this._replacePatern = options.replacePatern || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

_replacePatterns

Copy link
Contributor

Choose a reason for hiding this comment

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

Pas utile de mettre une valeur par défaut coûteuse à tester

@@ -31,6 +33,9 @@ class TextIndex {

find(terms, options = {}) {
let boosted = false;

terms = replaceAbbreviations(terms, this._replacePatern);
Copy link
Contributor

Choose a reason for hiding this comment

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

if (this._replacePatterns) {
  terms = replace(terms, this._replacePatterns);
}

const removeDiacritics = require('./removeDiacritics');

function replaceAbbreviations(search, abbreviations) {
const terms = removeDiacritics(search)
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi utilise removeDiacritics ici ?

function replaceAbbreviations(search, abbreviations) {
const terms = removeDiacritics(search)
.toLowerCase()
.replace(/"/g, '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Un commentaire est bienvenu

Copy link
Contributor

Choose a reason for hiding this comment

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

Je pense que tu fais trop de chose dans cette fonction replace. Mieux faut avoir plusieurs fonctions, les appeler dans le bon ordre et leur donner le nom adapté.

const terms = removeDiacritics(search)
.toLowerCase()
.replace(/"/g, '')
.replace(/-/g, ' ')
Copy link
Contributor

Choose a reason for hiding this comment

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

Idem

.replace(/-/g, ' ')
.split(' ');

if (terms.length <= 1) return search;
Copy link
Contributor

Choose a reason for hiding this comment

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

search est trop abstrait

Copy link
Contributor

Choose a reason for hiding this comment

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

Au lieu de faire toutes ces transformations pour rien, je ferai le test de la présence d'un espace dès le début.

On fait de l'auto-complétion, chaque miliseconde compte.

const expect = require('expect.js');
const replaceAbbreviations = require('../lib/searchableCollection/replaceAbbreviations');

describe.only('replaceAbbreviations()', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

only

});
});

describe('Patern is contained in a word', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Pattern

'st': 'saint',
'ste': 'sainte',
'cgne': 'campagne',
};
Copy link

@teleboas teleboas Feb 3, 2017

Choose a reason for hiding this comment

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

A rapprocher de la liste utilisée pour les noms de rues dans la BAN ?
https://github.com/etalab/ban-data/blob/master/data/abbrev.txt

@jdesboeufs
Copy link
Contributor

@odtvince j'avais regardé mais très peu sont utiles dans les faits, et potentiellement trompeuses pour les communes.
À voir à l'usage quand on aura du traitement par lot sur l'API.

@jdesboeufs jdesboeufs removed this from the Rentrée des classes milestone Jul 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mots contractés
3 participants