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

Fix check for python version on sc2json script #179

Closed
wants to merge 4 commits into from
Closed

Fix check for python version on sc2json script #179

wants to merge 4 commits into from

Conversation

manuelseeger
Copy link
Contributor

Python3 deprecates encoding option on json.dumps and defaults to UTF8.

Python3 deprecates encoding option on json.dumps and defaults to UTF8.
@manuelseeger
Copy link
Contributor Author

This was nonsense.

Scripts still errors for me though even with the TypeError. Is the executable delivered with the pip package out of date?

Python3 deprecates encoding option on json.dumps and defaults to UTF8.

The existing try didn't work since the plugin wrapper
doesn't call json.dumps until after except statement in line 41.
@manuelseeger manuelseeger reopened this Jan 8, 2023
@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

Can you please provide the stack trace for the TypeError?

@manuelseeger
Copy link
Contributor Author

The existing TypeError check doesn't seem to work since the plugin wrapper doesn't invoke json dumps until after the replay is being loaded in line 41

@manuelseeger manuelseeger changed the title Remove encoding option from JSON script. Fix check for python version on sc2json script Jan 8, 2023
@manuelseeger
Copy link
Contributor Author

The TypeError is thrown on factory.load_replay, not on factory.register_plugin, which is being checked.

replay_json = factory.load_replay(args.path[0])

@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

The TypeError is thrown on factory.load_replay

Can you please provide the stack trace?

@cclauss
Copy link
Collaborator

cclauss commented Jan 8, 2023

#163 (comment)

@manuelseeger
Copy link
Contributor Author

Traceback (most recent call last):
File "C:\Users\manuel\dev\sc2reader\sc2reader\scripts\sc2json.py", line 46, in
main()
File "C:\Users\manuel\dev\sc2reader\sc2reader\scripts\sc2json.py", line 41, in main
replay_json = factory.load_replay(args.path[0])
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 85, in load_replay
return self.load(Replay, source, options, **new_options)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 163, in load
return self._load(cls, resource, filename=filename, options=options)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\sc2factory.py", line 174, in load
obj = plugin(obj)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\plugins\utils.py", line 17, in call
return func(*args, **opt)
File "c:\users\manuel\dev\sc2reader\sc2reader\factories\plugins\replay.py", line 19, in toJSON
return json.dumps(obj, **options)
File "C:\Users\manuel.conda\envs\sc2reader\lib\json_init
.py", line 234, in dumps
return cls(
TypeError: JSONEncoder.init() got an unexpected keyword argument 'encoding'

args = parser.parse_args()

factory = sc2reader.factories.SC2Factory()
try:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change above is good.
The change below is not really required because the try / except block does the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I would leave the original try/except it would throw an AttributeError before the TypeError though if args.encoding doesn't exist

@StoicLoofah
Copy link
Collaborator

If python3 doesn't support encoding, should we just drop the encoding option entirely from this command?

@cclauss
Copy link
Collaborator

cclauss commented Jan 9, 2023

should we just drop the encoding option entirely from this command?

Yes, but we should also drop Python 2… #163 (comment)

@StoicLoofah
Copy link
Collaborator

should we just drop the encoding option entirely from this command?

Yes, but we should also drop Python 2… #163 (comment)

You already did a great job clearing out a bunch of Python 2 stuff in this PR #174 Is there more we need to do in the code to drop support, or is it just a config thing?

@cclauss
Copy link
Collaborator

cclauss commented Aug 30, 2023

@manuelseeger Can you please rebase this pull request or close it?

parser.add_argument(
"path",
metavar="path",
type=str,
nargs=1,
help="Path to the replay to serialize.",
)
if sys.version_info.major < 3:
Copy link
Collaborator

Choose a reason for hiding this comment

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

As of #174 Python 2 is no longer supported.

@manuelseeger manuelseeger closed this by deleting the head repository Dec 28, 2023
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