GoodClover / OSM-Discord-bot

Discord bot that works with OpenStreetMap data. (MAINTAINANCE MODE)
Other
10 stars 2 forks source link

Add support for other syntaxes to specify inline reference to node/way/relation #4

Closed kallejre closed 3 years ago

kallejre commented 3 years ago

At line 742, regex currently supports only formats like node/1234. Requesting feature to support phrases like nodes 1, 2, 5 and 6 or 3.

Half-tested solution ELM_INLINE_REGEX = rf"{SS}(node|way|relation)(?:s? |\/)({POS_INT}(?:(?:, | and | or | )(?:{POS_INT}))*){SE}. It might be good or bad idea to unify similar regex for lines 742, 743 and 744. New regex supports array separators ,, and and or while accepting old node/123 format.

https://github.com/GoodClover/OSM-Discord-bot/blob/1901b577845997e5a3d4af9f9fd069fda9cad58d/main.py#L742-L744

Additional group-based regex processing may need to be implemented in on_message(). Since regex doesn't support repeating capturing groups, entire list of element ID-s are matched as single group (e.g 1, 2, 5 and 6 or 3) and needs next regex to match each {POS_INT} to find element ID-s individually. For example line 755 becomes elms = [(elm[0], tuple(re.findall('\d+', elm[1]))) for elm in re.findall(ELM_INLINE_REGEX, msg.clean_content) instead of

https://github.com/GoodClover/OSM-Discord-bot/blob/1901b577845997e5a3d4af9f9fd069fda9cad58d/main.py#L755

PS. There's typo in readme. https://github.com/GoodClover/OSM-Discord-bot/blob/1901b577845997e5a3d4af9f9fd069fda9cad58d/README.md#L23

kallejre commented 3 years ago

Also line 772 needs for-loop to process forementioned change.

https://github.com/GoodClover/OSM-Discord-bot/blob/1901b577845997e5a3d4af9f9fd069fda9cad58d/main.py#L770-L774

GoodClover commented 3 years ago

I would rather not do this, as I think it would happen by accident too much. When talking about nodes you don't want it to link every time someone mentions it, that would just get annoying. Having the explicit / in there should avoid this.

RegEx not having repeatable capture groups is really annoying :/ but what you said should probably work.

kallejre commented 3 years ago

How about asking user confirmation by requiring them to react with magnifying glass emoji? I couldn't get your bot running so no pull request, but i tested most of functionality on my bot.

Lines 748-750:

### Inline linking ###
ELM_INLINE_REGEX = rf"{SS}(node|way|relation)(?:s? |\/)({POS_INT}(?:(?:, | and | or | )(?:{POS_INT}))*){SE}"
CHANGESET_INLINE_REGEX = rf"{SS}(changeset)(?:s? |\/)({POS_INT}(?:(?:, | and | or | )(?:{POS_INT}))*){SE}"
USER_INLINE_REGEX = rf"{SS}user\/[\w\-_]+{SE}"

Lines 770-771

    #### Inline linking ####
    # Find matches
    elms = [(elm[0], tuple(re.findall('\d+', elm[1]))) for elm in re.findall(ELM_INLINE_REGEX, msg.clean_content)
    changesets = [tuple(re.findall('\d+', elm[1])) for elm in re.findall(CHANGESET_INLINE_REGEX, msg.clean_content)

Asking user confirmation somewhere around line 779. Note needing asyncio module for error handling

    # Ask user confirmation by reacting with :mag_right: emoji.
    reaction_string='\U0001f50e'  # :mag_right:
    await msg.add_reaction(reaction_string)
    def check(reaction, user_obj):
        return user_obj == msg.author and str(reaction.emoji) == reaction_string
    try:
        reaction, user_obj = await client.wait_for('reaction_add', timeout=15.0, check=check)
    except asyncio.TimeoutError:  # User didn't respond
        await message.clear_reaction(reaction_string)
        return 
    else:  # User responded
        await message.clear_reaction(reaction_string)
    # Run queries

And finally replacing 785-795 with

        for elm_type, elm_ids in elms:
            for elm_id in elm_ids:
                try:
                    embeds.append(elm_embed(get_elm(elm_type, elm_id)))
                except ValueError:
                    pass

        for changeset_ids in changesets:
            for changeset_id in changeset_ids:
                try:
                    embeds.append(changeset_embed(get_changeset(changeset_id)))
                except ValueError:
                    pass

Although i do think last snippet needs some code deduplication.