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

Unify and translate conozco.py to en.US #9

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

Conversation

srevinsaju
Copy link
Member

  • Translate Espanol to English
  • Reorder imports
  • Flake8 Fixes
  • Correct DBus.LookupError
  • Normal termination of activity

I am planning to copy paste this commit on every conozco activity on sugarlabs.
Please check if it is right

* Translate Espanol to English
* Reorder imports
* Flake8 Fixes
* Correct DBus.LookupError
* Normal termination of activity
@srevinsaju
Copy link
Member Author

@quozl @chimosky Please review, however after this commit there are few advantages and disadvantages

Advantages

  • The DBusLookupError of all the activities will get fixed, because the same conozco.py will be replicated in other iknow* actvities
  • All the Espanol terms get translated to English
  • More whitespace and flake8 fixes
  • Normal termination of activities

Disadvantages

  • A git bisect might not be possible after the commit / or to find errors related to the same

Is it possible to maintain a single copy of conozco.py in another repository, so that it remains a unique file, similar to sugargame on sugarlabs/sugargame or, collabwrapper on sugarlabs/collabwrapper. This might help future contributors to fix and release bugs on all the iknow* activities by releasing and tagging conozco on the main repository

This is a suggesstion, please let me know of the possibility.


This PR needs to be tested extensively before its deployed on all the iknow* activities.

@quozl
Copy link
Contributor

quozl commented May 20, 2020

I'm fine with a complete change in principle. I think it is too large to review, and best path forward may be to increase the number of people testing. I'd rather not have yet another repository. 😁

@srevinsaju
Copy link
Member Author

@quozl Yes, it was complete rewrite of variable names as such. Feel free to close this PR, in case if its not necessary at the moment; If you would like me to replicate the same conozco.py in every iknow* activities, I could do that too so that all the activities which have diverged from the main conozco can receive all the bugfixes which each iknow* activity recieved on sugarlabs

conozco.py Outdated
import os.path
import configparser
import gettext
import imp
Copy link
Member

Choose a reason for hiding this comment

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

Use importlib as imp existed in python2 and no longer does.

Copy link
Contributor

Choose a reason for hiding this comment

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

python3 -c "import imp" seems to work for me on 3.6.9. Is there a specific version where it does not work?

Copy link
Member

Choose a reason for hiding this comment

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

Using 3.7.3 importing it throws a DeprecationWarning.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. A fix is in

https://github.com/sugarlabs/iknowEditor/blob/master/conozco.py#L31-L32

https://github.com/sugarlabs/iknowEditor/blob/master/conozco.py#L251

Seems a lot of work to go through just to keep the file in a non-module directory, otherwise a plain import would be fine. I wonder what was behind this design decision?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I will update accordingly

@chimosky
Copy link
Member

I'm fine with a complete change in principle. I think it is too large to review, and best path forward may be to increase the number of people testing. I'd rather not have yet another repository. grin

I agree, I haven't reviewed yet; just took a look.

@quozl
Copy link
Contributor

quozl commented May 21, 2020

@quozl Yes, it was complete rewrite of variable names as such. Feel free to close this PR, in case if its not necessary at the moment;

No need to close, it's better than what we have now.

If you would like me to replicate the same conozco.py in every iknow* activities, I could do that too so that all the activities which have diverged from the main conozco can receive all the bugfixes which each iknow* activity recieved on sugarlabs

You're welcome to do that, just that these should be considered as wide-ranging changes and won't necessarily get a detailed review. The activities are local, so a smaller user base. The iKnow Editor is the most important one to get right, and the generated code should look like that.

@srevinsaju
Copy link
Member Author

I tried to update the iknowEditor with a updated version of conozco, but as iknowEditor was not in a working condition in the first place, so I did not update it. The save function in the iknowEditor does not work at the moment or I have no clue where the file is saved.

@quozl
Copy link
Contributor

quozl commented May 21, 2020

Well, that's news to me too. No issues created or fixes proposed. You could debug the save function with pdb?

@srevinsaju
Copy link
Member Author

Sure, then I shall fix iknowEditor before other iknow* activities.

@srevinsaju
Copy link
Member Author

Wow!, The iknowEditor 's conozco did not load the imported module. I was trying to figure out why the patched conozco from iknowEditor did not load the imported module, and thats probably why it did not work

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.

None yet

3 participants