FM-17 / poglink

A bot that integrates the ARK Web API with Discord.
MIT License
12 stars 4 forks source link

travis rates urls #45

Closed travipross closed 2 years ago

travipross commented 2 years ago

Fixes #42. Should be merged after #40

Comma-separated lists were handled in the poglink.utils.setup_config function which is being used in the main.py module right now, but the same level of type-checking/parsing wasn't being done for arguments passed directly to the BotConfig class for instantiation.

travipross commented 2 years ago

We may want to handle the case where the user provides a list of strings

I would consider that invalid syntax that shouldn't be really expected by poglink, I'm afraid (note that it's a pyyaml error). The top level YAML structure is a map (key-value pairs), and the rates_urls key expects an array or a string (which may consist of a series of comma delimiters that your application is now smart enough to know how to handle).

There is no such construct within YAML as a bare "list of strings" that are separated by commas. It's either an array denoted by hyphen-prefixed newlines or by square brackets. The latter of those is likely what you intended to show here (reference).

Also worth noting in that case, the double quotes aren't really necessary either; YAML assumes string type by default and the quotes are mostly just good for escaping special characters or for readability.

So overall, you've got 3 options:

  1. hyphenated line items (real YAML)
  2. square brackets with comma separation (real YAML)
  3. single string with comma separation (not real YAML, but your app will know how to break down the string into its individual parts)

I wouldn't handle this error personally, as the pyYAML library is already raising a descriptive error indicating that some invalid syntax is being supplied

travipross commented 2 years ago

I wouldn't handle this error personally, as the pyYAML library is already raising a descriptive error indicating that some invalid syntax is being supplied

You know what, on 2nd thought, I think I misspoke. It wouldn't be a terrible idea to create a custom exception like ConfigReadError to re-raise after catching the above yaml error. I just think it still should be re-raised and left uncaught unless you would prefer a different behaviour when provided an invalid configuration file?

FM-17 commented 2 years ago

I wouldn't handle this error personally, as the pyYAML library is already raising a descriptive error indicating that some invalid syntax is being supplied

You know what, on 2nd thought, I think I misspoke. It wouldn't be a terrible idea to create a custom exception like ConfigReadError to re-raise after catching the above yaml error. I just think it still should be re-raised and left uncaught unless you would prefer a different behaviour when provided an invalid configuration file?

Yeah, I just meant that we should catch and log the error for their reference rather than letting it crash without explanation

travipross commented 2 years ago

Yeah, I just meant that we should catch and log the error rather than letting it crash without explanation

I'll add that in now real quick

github-actions[bot] commented 2 years ago

:tada: This PR is included in version 0.6.7 :tada:

The release is available on:

Your semantic-release bot :package::rocket: