Skip to content

Refactored the app.py code#11

Open
saaiiravi wants to merge 1 commit into
gauranshkumar:masterfrom
saaiiravi:sai
Open

Refactored the app.py code#11
saaiiravi wants to merge 1 commit into
gauranshkumar:masterfrom
saaiiravi:sai

Conversation

@saaiiravi

Copy link
Copy Markdown
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
Copy Markdown
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()

Comment thread src/app.py
return server


def upload_cc(c1):

Copy link
Copy Markdown
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.

Comment thread src/app.py
c1.warning("Please select the correct column for Email Id.")


def upload_bcc(c2):

Copy link
Copy Markdown
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

Comment thread src/app.py
c2.warning("Please select the correct column for Email Id.")


def add_attachments(cc, mailid, passwrd, server):

Copy link
Copy Markdown
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()

Comment thread src/app.py
attachments = st.file_uploader("Attachments:", accept_multiple_files=True)

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

Copy link
Copy Markdown
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.

Comment thread src/app.py
# Calling the core Mailer Class
mailer = Mailer(mailid, passwrd, server.lower())

if cc == [""]:

Copy link
Copy Markdown
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()

Comment thread src/app.py
""", unsafe_allow_html=True)


def main():

Copy link
Copy Markdown
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

saaiiravi commented Oct 11, 2021

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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