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

revert commits that attempted to solve tokenization bug #122

Merged
merged 3 commits into from
Jan 16, 2019

Conversation

lgessler
Copy link
Collaborator

@amir-zeldes, do these code changes look good to you? I made them temporarily in prod and it resolved the bug (should be obvious what was happening looking at the diff--we were overwriting the new text with the old stuff right after getting it from the service). I'm worried about regressions, though, esp. with the removal of the line that was doing some encoding for reasons I don't understand.

@lgessler
Copy link
Collaborator Author

Hold on, this change breaks saving for some reason. Investigating...

@lgessler lgessler closed this Jan 16, 2019
@lgessler lgessler reopened this Jan 16, 2019
@lgessler lgessler changed the title don't overwrite text content if it exists already when saving revert commits thqt attempted to solve tokenization bug Jan 16, 2019
@lgessler lgessler changed the title revert commits thqt attempted to solve tokenization bug revert commits that attempted to solve tokenization bug Jan 16, 2019
@amir-zeldes
Copy link
Contributor

Looks like this is having some side-effects in production, probably due to the encoding issue(?) - here's a user report:

I'm working on updating some division errors and metadata in Johannes for Carrie, but I'm getting the white screen Carrie references in the email below, and I'm unable to commit changes to Github. It looks like it's related to unicode issues in the DB. Carrie, the other documents should be fine to work on, and I'll update the metadata as soon as I can commit. I see that Luke is working on the tokenization issue in Gitdox, but it also looks as if the validator can't find the xml schema. I'm not sure if this is related

As you resolve the tokenizer issue, can you also clone a document from johannes.canons, test that saving and committing work, and resolve if not? Thanks!

@lgessler
Copy link
Collaborator Author

lgessler commented Jan 16, 2019

This is the error I'm seeing:

/var/www/html/gitdox/scriptorium/modules/gitdox_git.py in push_update_to_git(username='lgessler', token='ceea4649f15f5f6707bf5385c3cd44bd2c007e6d', path=u'new_document.xml', account=u'account', repo=u'repo_name', message='test')

27                 with open(prefix+file_info, 'rb') as fd:
28                         contents = fd.read()
29                 contents_object = repository.contents(file_info)
30                 if contents_object: #this file already exists on remote repo
31                         #update
contents_object undefined, repository = None, repository.contents undefined, file_info = u'new_document.xml'

This is happening even without the changes in this PR (I deployed the version before it on scriptorium) so it appears unrelated. My best guess right now is that it might have to do with how github3.py is at 0.9.6 instead of 0.9.3. I can't change the version since I'm not a sudoer, though. Whatever it is, since it's happening without the changes in this PR, I'm pretty sure it's unrelated. Will look into this more this afternoon

@lgessler
Copy link
Collaborator Author

@ctschroeder @amir-zeldes (and @whoever wrote the snippet above), could you please share more details about which document, corpus, repo, etc. you were trying to commit with? Committing appears to be working correctly, but I think it might choke if your corpus name, repo name, and/or github credentials are wrong.

@amir-zeldes amir-zeldes merged commit 91ff56c into gucorpling:dev Jan 16, 2019
@amir-zeldes
Copy link
Contributor

OK, @eplatte @ctschroeder as far as I can see everything is now resolved, and tokenization works as expected (tested on johannes.canons.FA143-158). If there is still anything that doesn't work please let us know!

@ctschroeder
Copy link
Collaborator

@lgessler @amir-zeldes everything looks good! I even get the commit message again!!! Thank you!

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