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

GUI and Improved Documentation #32

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

GUI and Improved Documentation #32

wants to merge 11 commits into from

Conversation

ntjennings1
Copy link

No description provided.


### Missing Aysnchronous attribute

![Example Error](https://github.com/njenn001/CarbonDate/blob/master/images/Error.jpg)
Copy link
Member

Choose a reason for hiding this comment

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

Make the image source relative, so it does not point to a specific fork.

Suggested change
![Example Error](https://github.com/njenn001/CarbonDate/blob/master/images/Error.jpg)
![Example Error](images/Error.jpg)


For quick installation and running purposes simply comment out the use of the server import in main.py.

![Example Solution](https://github.com/njenn001/CarbonDate/blob/master/images/solution.JPG)
Copy link
Member

Choose a reason for hiding this comment

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

Make the image source relative, so it does not point to a specific fork.

Suggested change
![Example Solution](https://github.com/njenn001/CarbonDate/blob/master/images/solution.JPG)
![Example Solution](images/solution.JPG)

```
* Use the small tkinter window to carbon date URLs

* ![GUI Figure](https://github.com/njenn001/CarbonDate/blob/master/images/GuiFig.png)
Copy link
Member

Choose a reason for hiding this comment

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

Make the image source relative, so it does not point to a specific fork.

Suggested change
* ![GUI Figure](https://github.com/njenn001/CarbonDate/blob/master/images/GuiFig.png)
* ![GUI Figure](images/GuiFig.png)

@@ -1,5 +1,5 @@
{
"AccessToken":["YourBitlyAccessTokenHere"],
"AccessToken":["Yf7d318a38ee40a5049861a32814bb1b0fb78cb8b"],
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to provide an access token in the source? This token will perhaps be revoked by GitHub. It is better to tell user how to create a token and then they can use that instead.

@@ -86,7 +86,7 @@ def run(self, args, **kwargs):
if args.local_uri:
url = args.local_uri
else:
url = kwargs['url']
url = 'https://www.cs.odu.edu/'
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason of this change?

def __init__(self, parent, controller):
def on_entry_click(event):
"""function that gets called whenever entry is clicked"""
if URIentry.get() == 'https://cs.odu.edu/':
Copy link
Member

Choose a reason for hiding this comment

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

Adding hardcoded values like this is not a good practice. If this were to be used as the sample URI, a constant should be defined at the top of the file or better yet, add this in the config file to reuse everywhere.

main.py Outdated
elif args.server:
server.start(args, cfg, mod)
#elif args.server:
# server.start(args, cfg, mod)
Copy link
Member

Choose a reason for hiding this comment

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

Wait a second, are you completely removing the server functionality and making it a GUI-only application? I am afraid, that I cannot merge this PR here, if this is the case.


![Example Solution](https://github.com/njenn001/CarbonDate/blob/master/images/solution.JPG)

### Missing/Broken Modules
Copy link
Member

Choose a reason for hiding this comment

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

Are these modules broken? Open separate issues with details to either fix or remove them.

import json
import time
import tkinter as tk
Copy link
Member

Choose a reason for hiding this comment

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

Need to update requirements.txt to include tkinter and gui.


### Missing Aysnchronous attribute

![Example Error](https://github.com/njenn001/CarbonDate/blob/master/images/Error.jpg)
Copy link
Member

Choose a reason for hiding this comment

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

I'm kind of curious where this error stems from. I don't have issues with the docker hub image.

Maybe there is a Python version and library version mismatch going on? The docker image is python 3.7 with the following library versions:

$ docker run -it --rm oduwsdl/carbondate bash
root@4d968fac6b3c:/usr/src/app# pip3 list --format freeze
atomicwrites==1.1.5
attrs==18.1.0
beautifulsoup4==4.6.3
certifi==2018.8.13
chardet==3.0.4
idna==2.7
lxml==4.2.4
more-itertools==4.3.0
pip==18.0
pluggy==0.7.1
py==1.5.4
pytest==3.7.1
python-dateutil==2.7.3
requests==2.19.1
requests-file==1.4.3
setuptools==40.0.0
six==1.11.0
surt==0.3.0
tldextract==2.2.0
tornado==5.1
urllib3==1.23
wheel==0.31.1

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.

3 participants