Skip to content
This repository has been archived by the owner on May 2, 2022. It is now read-only.

add functionality for ascii-key input through keyfile #42

Merged
merged 3 commits into from
Oct 10, 2021
Merged

add functionality for ascii-key input through keyfile #42

merged 3 commits into from
Oct 10, 2021

Conversation

ckosa
Copy link
Contributor

@ckosa ckosa commented Oct 7, 2021

remove hardcoded ASCII_CHAR to replace with input functionality
add parse argument to check for keyfile path
read ascii index from input file

@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 27 12
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
3 14 32

@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 27 12
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
3 14 32

@zero-to-mastery-bot
Copy link

⚠️ MERGE CONFLICT DETECTED!

@ckosa A possible conflict has been detected, you will need to resolve this before your pull request can be merged. The most common reason conflicts occur, is when the contributor does not run git pull origin master before pushing their new changes.

Before we can merge the code, you will need to resolve the conflict, there are tons of guides on Google and Youtube to help you out. If you get stuck ask over on Discord.

@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 26 11
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
3 14 32

1 similar comment
@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 26 11
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
3 14 32

@ndeoligence
Copy link
Contributor

@ckosa this commit breaks the webapp. It imports and uses the method: handle_image_conversion.

@ndeoligence ndeoligence added the bug Something isn't working label Oct 9, 2021
@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 26 11
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
3 14 36

ndeoligence
ndeoligence suggested changes Oct 9, 2021
View reviewed changes
Copy link
Contributor

@ndeoligence ndeoligence left a comment

Choose a reason for hiding this comment

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

Please take a look at the suggestions on relevant snippets.

For the breaking change, I suggest adding a constant near the top of app.py that has the path to default key and then pass it to the method that requires it.

akey.txt Outdated Show resolved Hide resolved
akey2.txt Outdated Show resolved Hide resolved
community_version.py Show resolved Hide resolved
@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 31 16
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
5 14 36

@ckosa
Copy link
Contributor Author

ckosa commented Oct 10, 2021 •
edited
Loading

I added default directories to the keyfile for both gui.py and app.py.
I was able to successfully test gui.py.

However, I need additional guidance/documentation on how to correctly initialize the webapp for app.py. Even when running from master I encounter errors due to not having the root directory './webapp/*'. I used both flask run and python app.py to initialize the webapp. Are there other steps necessary for the webapp to initialize the default directory?

Update(2021-10-09_21:05ct): I manually created the directories ./webapp/* and was able to successfully execute the app.

@ndeoligence ndeoligence mentioned this pull request Oct 10, 2021
Closed
@ndeoligence
Copy link
Contributor

@ckosa Good job on catching the GUI reference to the same method. And thanks for the feedback on testing. I've created an issue out of what you said about the webapp/uploads directories.

@ndeoligence ndeoligence removed the bug Something isn't working label Oct 10, 2021
@zero-to-mastery-bot
Copy link

🙏 Thanks for your pull request @ckosa, The team will now review and merge this request. In the mean time why not check out some of the other opensource projects available, contributions are greatly appreciated!

Some of the most popular are

PR Statistics

#️⃣ PR Number: ➕ Line Additions: 🗑️ Line Deletions:
42 31 16
📑 Files Changed: ⭐ Repo Stars: 🔱 Total Forks:
5 14 36

ndeoligence
ndeoligence reviewed Oct 10, 2021
View reviewed changes
akey.txt
@@ -0,0 +1 @@
#?%.S=.*:""@
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that the "" need to be replaced by a ,

@ndeoligence ndeoligence mentioned this pull request Oct 10, 2021
Closed
@ndeoligence ndeoligence merged commit 03f5f3c into zero-to-mastery:master Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ndeoligence ndeoligence ndeoligence approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ckosa @ndeoligence @deepaksaipendyala