Skip to content

Use the path parameter of encode/decode again to be future-proof#223

Closed
syssi wants to merge 1 commit intorytilahti:masterfrom
syssi:feature/construct-encode-decode-path
Closed

Use the path parameter of encode/decode again to be future-proof#223
syssi wants to merge 1 commit intorytilahti:masterfrom
syssi:feature/construct-encode-decode-path

Conversation

@syssi
Copy link
Copy Markdown
Collaborator

@syssi syssi commented Feb 14, 2018

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 67.951% when pulling 74e9bed on syssi:feature/construct-encode-decode-path into 26fc798 on rytilahti:master.

2 similar comments
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 67.951% when pulling 74e9bed on syssi:feature/construct-encode-decode-path into 26fc798 on rytilahti:master.

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 67.951% when pulling 74e9bed on syssi:feature/construct-encode-decode-path into 26fc798 on rytilahti:master.

@rytilahti
Copy link
Copy Markdown
Owner

I suppose the question to ask here is whether we are more afraid of potential silent errors (which we can handle by checking against incoming kwargs) or changes in the interface used by construct. Maybe keeping the path as it is, and adding **kwargs in case the interface gets extended at some point, to avoid crashing it all immediately?

@yawor
Copy link
Copy Markdown
Contributor

yawor commented Feb 14, 2018

There's still an open issue #224 when using Construct > 2.9.28.

@arekbulski
Copy link
Copy Markdown
Contributor

arekbulski commented Feb 15, 2018

Instead of crashing "immediately" due to error explictly related to Adapter code, it will fail "later" due to unknown reasons that dont flag construct lines of code in any way. You might as well wrap you entire code in try/catch and ship it.

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.

5 participants