Skip to content

Conversation

@nrontsis
Copy link
Contributor

@nrontsis nrontsis requested a review from a team March 21, 2021 21:50
Copy link
Collaborator

@vdonato vdonato left a comment

Choose a reason for hiding this comment

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

Thanks a ton for the contribution @nrontsis! For the most part, this looks great to me -- I had a few comments that are just minor points.

We generally require two reviewers for community contributions, so on my end I'll bug some people to get another pair of eyes on this PR.

@tconkling tconkling self-requested a review March 23, 2021 18:12
Copy link
Contributor

@tconkling tconkling left a comment

Choose a reason for hiding this comment

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

Looks good to me, once @vdonato's comments are addressed! Thanks for this, @nrontsis!

@nrontsis
Copy link
Contributor Author

Thanks guys! I have now addressed @vdonato's feedback, I think 🚀

Copy link
Collaborator

@vdonato vdonato 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 @nrontsis for both fixing #3000 and cleaning up the code in local_sources_watcher! 🙇

I think this is good to merge once @tconkling adds his official stamp of approval

@tconkling tconkling merged commit 6c09be6 into streamlit:develop Mar 24, 2021
@tconkling
Copy link
Contributor

Merged. Thanks, @nrontsis, this is great work.

tconkling added a commit that referenced this pull request Mar 24, 2021
# By Vincent Donato (3) and Nikitas Rontsis (1)
# Via GitHub
* develop:
  Have theme hashes differentiate between being unset and the hash for null (#2993)
  Improve module reloading (#3001)
  Add tests to improve coverage around config options set via flag (#2991)
  Add snippets about CSS vars and minimum streamlit-component-lib versions (#3008)

# Conflicts:
#	frontend/src/App.tsx
vdonato added a commit to vdonato/streamlit that referenced this pull request Mar 31, 2021
I ran into some trouble trying to call st.warning outside of code run by
the ScriptRunner, which made me remember that some code changed in a
recent PR (streamlit#3001) did just that.

I double-checked that this code indeed doesn't work as calls that
end up sending Delta protos rely on a ReportContext, which isn't
available in the LocalSourcesWatcher.

Since we can't present the warning in Streamlit itself (which would be
ideal), I just downgraded this to be a warning log for now.
vdonato added a commit that referenced this pull request Apr 2, 2021
I ran into some trouble trying to call st.warning outside of code run by
the ScriptRunner, which made me remember that some code changed in a
recent PR (#3001) did just that.

I double-checked that this code indeed doesn't work as calls that
end up sending Delta protos rely on a ReportContext, which isn't
available in the LocalSourcesWatcher.

Since we can't present the warning in Streamlit itself (which would be
ideal), I just downgraded this to be a warning log for now.
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.

Implicit namespace packages are not detected by file watchers

3 participants