Skip to content

Conversation

@sfc-gh-mnowotka
Copy link
Contributor

@sfc-gh-mnowotka sfc-gh-mnowotka commented Mar 13, 2023

📚 Context

This change enables checking for unused variables in the Streamlit codebase. Checking for unused variables is useful when refactoring - for example, when splitting a function into two, we can make sure that all parameters have been moved correctly. As we already have some unused vars in our code, this PR removes them to ensure that the linter passes the checks introduced.

  • What kind of change does this PR introduce?

    • Bugfix
    • Feature
    • Refactoring
    • Other, please describe:

🧠 Description of Changes

  • Add bullet points summarizing your changes here

    • This is a breaking API change
    • This is a visible (user-facing) change

Revised:

Insert screenshot of your updated UI/code here

Current:

Insert screenshot of existing UI/code here

🧪 Testing Done

  • Screenshots included
  • Added/Updated unit tests
  • Added/Updated e2e tests

🌐 References

Does this depend on other work, documents, or tickets?

  • Issue: Closes #XXXX

Contribution License Agreement

By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-mnowotka sfc-gh-mnowotka added the security-assessment-completed Security assessment has been completed for PR label Mar 13, 2023
@sfc-gh-mnowotka sfc-gh-mnowotka marked this pull request as ready for review March 13, 2023 10:31
@sfc-gh-mnowotka sfc-gh-mnowotka marked this pull request as draft March 13, 2023 10:32
@sfc-gh-mnowotka sfc-gh-mnowotka marked this pull request as ready for review March 13, 2023 10:56
Copy link
Contributor

@sfc-gh-tszerszen sfc-gh-tszerszen left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I did not expect so much unused vars, thanks Michał! 🥇

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.

This is great!

# Conflicts:
#	frontend/src/hocs/withHostCommunication/withHostCommunication.test.tsx
@sfc-gh-mnowotka sfc-gh-mnowotka merged commit 7fc3dd3 into develop Mar 13, 2023
tconkling added a commit to tconkling/streamlit that referenced this pull request Mar 17, 2023
* develop: (48 commits)
  ForwardMessageCache: interface-ize endpoint (streamlit#6326)
  Add an event handler for popstate event to reflect user's back button being clicked (streamlit#6271)
  Cypress Upgrade to 9.7.0 (streamlit#6322)
  Remove default values from internal class reprs
  FileUploadClient: interface-ize endpoint (streamlit#6323)
  Fix issue streamlit#6310 (streamlit#6321)
  Make using @ts-ignore illegal (streamlit#6314)
  Add nightly-preview deploy to nightly build (streamlit#6306)
  Minor documentation fixes for cache storage (streamlit#6222)
  Add react-dropzone prop to address disabled file type behavior (streamlit#6315)
  Add timeout to label-markdown spec (streamlit#6312)
  ascii diagrams for CacheStorage* protocols, and for LocalDiskCacheStorage* concrete implementation (streamlit#6220)
  Bump @sideway/formula from 3.0.0 to 3.0.1 in /frontend (streamlit#6309)
  Bump webpack from 5.75.0 to 5.76.0 in /frontend (streamlit#6313)
  Replace @ts-ignore with @ts-expect-error and remove unused directives (streamlit#6308)
  Up version to 1.20.0 (streamlit#6284)
  Pull ComponentRegistry's URL generation into an interface (streamlit#6307)
  Add st.divider (streamlit#6178)
  fix: upgrade multiple dependencies with Snyk (streamlit#6262)
  Activate the no-unused-vars rule in eslint. (streamlit#6300)
  ...
@sfc-gh-kmcgrady sfc-gh-kmcgrady deleted the feature/no-unused-vars branch October 5, 2023 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

security-assessment-completed Security assessment has been completed for PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants