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

Refactored the app.py code #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

saaiiravi
Copy link
Contributor

Hi,

I've refactored it. Please review and let me know if there are any other changes to be made.

@gauranshkumar gauranshkumar linked an issue Oct 10, 2021 that may be closed by this pull request
@gauranshkumar
Copy link
Owner

Hi @saaiiravi, I have reviewed your code and that's good that you have used a functional approach but Streamlit is a very top-down approach and line-by-line execution-based API. The code here is actually breaking the application and generating some random Operations. I would recommend you to please go through the current stable code and make functions of only required features rest all can be managed in an def main()

return server


def upload_cc(c1):
Copy link
Owner

Choose a reason for hiding this comment

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

This can be easily managed in main() as c1 is a column of streamlit grid.

c1.warning("Please select the correct column for Email Id.")


def upload_bcc(c2):
Copy link
Owner

Choose a reason for hiding this comment

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

The same goes for this

c2.warning("Please select the correct column for Email Id.")


def add_attachments(cc, mailid, passwrd, server):
Copy link
Owner

Choose a reason for hiding this comment

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

no need for cc argument as all of this is handled in main()

attachments = st.file_uploader("Attachments:", accept_multiple_files=True)

# Calling the core Mailer Class
mailer = Mailer(mailid, passwrd, server.lower())
Copy link
Owner

Choose a reason for hiding this comment

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

mailer is the core class it can be either called in the mail() or main() depending on how you structure it.

# Calling the core Mailer Class
mailer = Mailer(mailid, passwrd, server.lower())

if cc == [""]:
Copy link
Owner

Choose a reason for hiding this comment

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

this check needs to be done in main()

""", unsafe_allow_html=True)


def main():
Copy link
Owner

Choose a reason for hiding this comment

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

try to manage the StreamlitUI Code in main() and all logic in separate functions for better control of Streamlit UI Components.

@saaiiravi
Copy link
Contributor Author

saaiiravi commented Oct 11, 2021

hi @gauranshkumar,

I've tested the code. It worked fine for me. If you can tell me what parts of the code is breaking, It'll be easier for me to do changes suggested by you and test.

Also, could you give more clarity on why you're asking to move certain piece of code to main. Calling the function and putting them in the main function is ideally the same right. It didn't make sense to me. If you can explain, I will be able to understand better.

@gauranshkumar
Copy link
Owner

Hi @saaiiravi, answering your first question, i.e where the code breaks, so actually the app runs without raising any error or warning but if you look at it carefully, as soon as we start the streamlit server the mail sending function is called automatically without pressing the sent button and it shows a notification that mail is sent successfully whereas, in reality, it isn't so it's creating a hidden bug. Also, there is an issue in rendering the subheaders as some don't render as markdown rather they are displayed as normal text.

@gauranshkumar
Copy link
Owner

Secondly, I agree that doing everything in the main() is ideally the same as writing it without function and that's the problem of Streamlit API as it is intended for direct line-by-line use, If we pass the Streamlit Objects in custom functions chances are that it won't refer to the same object due to indirect reference to the memory location of the objects.

@gauranshkumar
Copy link
Owner

I hope that makes some sense to you, so the idea here is to not just functionalize the code but rather make it less redundant and improve the quality.

@saaiiravi
Copy link
Contributor Author

It makes sense. So what is the exact feature you want me to implement in a function?
Because from my understanding, you want most of the code in main().

@saaiiravi
Copy link
Contributor Author

@gauranshkumar if you can tell me, what is the exact feature you want outside of main. I can finish It off.

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.

Refactoring the UI Code
2 participants