Skip to content

Conversation

@jrhone
Copy link
Contributor

@jrhone jrhone commented Mar 18, 2020

Issue #1189

@jrhone jrhone requested a review from a team as a code owner March 18, 2020 21:01
Copy link
Contributor

@tvst tvst left a comment

Choose a reason for hiding this comment

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

Approved! But please resolve comments.

self.update(h, obj.tell())
return h.digest()

elif isinstance(obj, Pattern):
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I didn't think this would work... I thought the typing module was supposed to be used only for type hints.

But either way, I think it would be clearer to say isinstance(obj, re.Pattern) instead.

Copy link
Contributor Author

@jrhone jrhone Mar 25, 2020

Choose a reason for hiding this comment

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

Tried something like that initially but comparing to typing.Pattern seems to be the recommended way to go about things.

>>> re.Pattern
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module 're' has no attribute 'Pattern'

h = hashlib.new("md5")
self.update(h, obj.pattern)
self.update(h, obj.flags)
return h.digest()
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just return b"%s:%s" % (obj.pattern, obj.flags) here since it gets md5'ed inside update()

Copy link
Contributor Author

@jrhone jrhone Mar 26, 2020

Choose a reason for hiding this comment

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

Perhaps it should be return self.to_bytes([obj.pattern, obj.flags])?

Your suggestion also works but we only use %s:%s when we prefix with the type at the moment. The current pattern is to recurse into to_bytes with some more primitive data.

@jrhone jrhone merged commit dd85245 into streamlit:develop Mar 26, 2020
tconkling added a commit that referenced this pull request Mar 30, 2020
* develop:
  Release 0.57.1 - Fixes SessionInfo alert bug (#1270)
  Py2k Elimination (phase 2): Scrubbing all the py2/3 compatibility clauses (#1177)
  Changing selectbox filtering as case insensitive (#1269)
  Moving st_in_cache_warning from flaky to stable (#1267)
  DataFrame | Fixing column width behavior (#1258)
  Release 0.57 (#1266)
  Fix bug where script was executing twice on first run. (#1263)
  Add default hash func for regex patterns (#1232)
  Updated readme.md help people find msft installation instructions.
  MediaFileManager: fix "Content-Type" header
tconkling added a commit to tconkling/streamlit that referenced this pull request Mar 30, 2020
* feature/plugins:
  Release 0.57.1 - Fixes SessionInfo alert bug (streamlit#1270)
  Py2k Elimination (phase 2): Scrubbing all the py2/3 compatibility clauses (streamlit#1177)
  Changing selectbox filtering as case insensitive (streamlit#1269)
  Moving st_in_cache_warning from flaky to stable (streamlit#1267)
  DataFrame | Fixing column width behavior (streamlit#1258)
  Release 0.57 (streamlit#1266)
  Fix bug where script was executing twice on first run. (streamlit#1263)
  Add default hash func for regex patterns (streamlit#1232)
  Updated readme.md help people find msft installation instructions.
  MediaFileManager: fix "Content-Type" header
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.

2 participants