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

Add /price command #167

Merged
merged 3 commits into from
Sep 24, 2022
Merged

Add /price command #167

merged 3 commits into from
Sep 24, 2022

Conversation

AlphaKretin
Copy link
Contributor

@AlphaKretin AlphaKretin commented Sep 24, 2022

Based on YGOPRODeck implementation
@coveralls
Copy link

coveralls commented Sep 24, 2022

Pull Request Test Coverage Report for Build 3117021257

  • 1 of 20 (5.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.6%) to 59.211%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils.ts 1 20 5.0%
Totals Coverage Status
Change from base Build 3087306250: -1.6%
Covered Lines: 388
Relevant Lines: 658

💛 - Coveralls

src/commands/price.ts Outdated Show resolved Hide resolved
src/commands/price.ts Outdated Show resolved Hide resolved
src/commands/price.ts Outdated Show resolved Hide resolved
src/commands/price.ts Outdated Show resolved Hide resolved
More accurate latency times, nomenclature improvements
@AlphaKretin
Copy link
Contributor Author

oh and of course as this is merged, we have to add the price api url to the environment

@kevinlul
Copy link
Contributor

Now that I think of it, I actually don't think it needs to be there because this is tailored to a specific API and it was already in the open before: https://github.com/AlphaKretin/ygoprodeck-tag-bot/blob/master/config.json#L13

@kevinlul kevinlul changed the title Add /price Slash Command Add /price command Sep 24, 2022
@AlphaKretin
Copy link
Contributor Author

Now that I think of it, I actually don't think it needs to be there because this is tailored to a specific API and it was already in the open before: https://github.com/AlphaKretin/ygoprodeck-tag-bot/blob/master/config.json#L13

It doesn't need to be secret, but it's consistent with how we handle our other APIs/URLs. I would also object to hardcoding it in the command file, as it may be used later to add price data to card embeds.

@kevinlul
Copy link
Contributor

Wouldn't we move the entire getPrice function out then, which contains the URL?

@AlphaKretin
Copy link
Contributor Author

Wouldn't we move the entire getPrice function out then, which contains the URL?

...True. In that case, if that's how you want to do it I don't necessarily object, but I do lightly prefer the current state.

@kevinlul
Copy link
Contributor

Let's inline it. It's consistent with /yugipedia and /ygoprodeck.

Copy link
Contributor

@kevinlul kevinlul left a comment

Choose a reason for hiding this comment

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

🚀 💸

@kevinlul kevinlul merged commit b94c994 into master Sep 24, 2022
@kevinlul kevinlul deleted the price branch September 24, 2022 04:34
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.

Price information
3 participants