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

Makman2/more issue formats #297

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

Conversation

Makman2
Copy link
Member

@Makman2 Makman2 commented Oct 2, 2017

No description provided.

... to be defined inside code easily by using custom parser functions.
@Makman2
Copy link
Member Author

Makman2 commented Oct 2, 2017

Hmm it seems to be more complicated than expected to get the room name...

roomname_match = re.fullmatch(
roomname_rgx, msg.frm.room.idd, re.IGNORECASE)

if roommatch is None:
Copy link
Member

Choose a reason for hiding this comment

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

You mean roomname_match, right?

if roommatch is None:
return None

return roommatch.group(1), roommatch.group(2), m.group(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

Copy link
Member

Choose a reason for hiding this comment

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

There is no m in this scope :/

Copy link
Member Author

Choose a reason for hiding this comment

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

ah yeah this has to be the very first match variable from above, will correct it :3
(you already see that my tests aborted above because accessing the room name raised an exception :D)


roomname_rgx = r'(.+?)/(.+)'
roomname_match = re.fullmatch(
roomname_rgx, msg.frm.room.idd, re.IGNORECASE)
Copy link
Member

Choose a reason for hiding this comment

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

msg.frm.room.uri instead of msg.frm.room.idd

Copy link
Member

Choose a reason for hiding this comment

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

However, for uri, you'll find that many coala room URIs are of coala/coala/<name> type

Copy link
Member Author

Choose a reason for hiding this comment

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

is that for custom rooms? Or do even repo-related rooms have a name like e.g. coala/coala/coala-bears?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, room itself does not exist inside the tests, I'm not sure how to mock it correctly, as this field seems to be backend-dependent.

... of the form `#1234`.

Closes #167
Copy link
Member

@jayvdb jayvdb left a comment

Choose a reason for hiding this comment

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

a messy PR. failing and merge conflicts. And yet has good easy to merge stuff in it.

@Makman2 , perhaps pull out the easy stuff so we can merge it. Or create issues and other people can solve them.

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

Successfully merging this pull request may close these issues.

4 participants