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

Py3 support + changes for new RBFW and SeleniumLibrary by aaltat #231

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

Conversation

idxn
Copy link

@idxn idxn commented Feb 16, 2019

Move to Python3. Drop support of python2 because python2 will be end of life in the year of 2019 so it would be better to move forward.

  • convert to py3 using 2to3 script
  • use the latest RBFW and SeleniumLibrary for unit/acceptance test
  • Fix some of the typos

@idxn
Copy link
Author

idxn commented Feb 16, 2019

Strange. The unittest on my windows machine has passed. I'll go checking with OSX again

@aaltat
Copy link
Collaborator

aaltat commented Feb 17, 2019

Well, it does look pretty good. There is only one failure in the Python 3.6.

@andriyko should we give him the push rights or what we should do?

Copy link
Owner

@andriyko andriyko left a comment

Choose a reason for hiding this comment

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

@idxn , could please update the PR description giving short answers to these questions: what? why? how? For example:
What?
Move to Python3. Drop support of Python2.

Why?
Because nobody uses Python2 anymore )

How?

  • change1
  • change2
  • etc

Thank you for your contribution!

Robot.sublime-settings Outdated Show resolved Hide resolved
command_helper/get_keyword.py Outdated Show resolved Hide resolved
command_helper/workspace_objects.py Outdated Show resolved Hide resolved
commands/query_completions.py Outdated Show resolved Hide resolved
dataparser/data_queue/scanner.py Show resolved Hide resolved
dataparser/parser_utils/util.py Outdated Show resolved Hide resolved
@aaltat
Copy link
Collaborator

aaltat commented Feb 21, 2019

I had one comment. In some point it would be vice to fix the tests. too

@idxn
Copy link
Author

idxn commented Feb 22, 2019

Sure, i'll try to get it fixed

idxn and others added 2 commits February 22, 2019 15:20
@idxn
Copy link
Author

idxn commented Feb 25, 2019

@andriyko @aaltat Do I need to get all the unittest fixed or just the one in py3.6 to get this pull merged?

@idxn
Copy link
Author

idxn commented Feb 25, 2019

@aaltat @andriyko I have taken a look and find my suspect. In testdata, resource file importing in 'test\resource\test_data\real_suite\resource\resource1\real_suite_resource.robot' does not exist in the project as follow:

Resource ../../resource2/real_suite_resource.robot

If it's intended to, the reason it fails on unix based system is that the order of the queue list is not the same as windows. In windows, it's like this:

[SeleniumLibrary, xxx, ../../resource2/real_suite_resource.robot]

On the other hand, in unix based system, it's like this:

[ ../../resource2/real_suite_resource.robot, xxx, SeleniumLibrary]

Currently, our implementation will raise an exception if it cannot find the file so it won't parse any files in the latter. It causes our unit test to fail. If we intends to use the test data like this for specific reason, I'll need to make the changes to continue parse the file in the queue so the test_scanner.TestScanner.test_queue_populated unittest will pass and it quite makes sense for me that the scanner should not stop a scanning process because of non-existing file.

@idxn
Copy link
Author

idxn commented Feb 26, 2019

Please ignore the first comment. It misunderstood something :( I'll take a look again.

@aaltat
Copy link
Collaborator

aaltat commented Feb 26, 2019

I am having a flu and laying in the bed. I will take a look later.

@idxn
Copy link
Author

idxn commented Feb 26, 2019

Take some rest. I'm not in rush though. I'll try to get it resolve on my own.

command_helper/get_keyword.py Outdated Show resolved Hide resolved
command_helper/get_keyword.py Show resolved Hide resolved
@@ -3,10 +3,10 @@

try:
from parser_utils.file_formatter import lib_table_name
from noralize_cell import get_data_from_json
from normalize_cell import get_data_from_json
except ImportError:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps not in this commit, but if you are going to support only Python 3, then these type of try/except are not needed.

Copy link
Author

@idxn idxn Mar 3, 2019

Choose a reason for hiding this comment

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

Ok. I think I would get this merged first and do the refactoring again in the coming pull request.

@aaltat
Copy link
Collaborator

aaltat commented Mar 1, 2019

Well, I am pretty happy with the changes. Although looking in to why those test fails would be good to go .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants