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 #252: lookForAtArea matrix result type. #253

Merged
merged 4 commits into from
Jan 6, 2024

Conversation

zyzyzyryxy
Copy link
Contributor

Brief Description

Fix incorrect typing for return value of room.lookForAtArea matrix form (isArray = false)

Checklists

  • Test passed
  • Coding style (indentation, etc) - hope so, I let workpace-defined formatter do its job
  • Edits have been made to src/ files not index.d.ts
  • Run npm run dtslint to update index.d.ts

@Jomik
Copy link
Contributor

Jomik commented Jan 6, 2024

We are missing tests for this change, it is a bit hard to see what this actually accomplishes in the context of this PR.

@zyzyzyryxy
Copy link
Contributor Author

We are missing tests for this change, it is a bit hard to see what this actually accomplishes in the context of this PR.

I thought I did add the tests, but maybe I am missing something about the project structure? Thought dist/screeps-tests.ts contains tests, I added lines 919 - 921 with a test from the issue reported and fixed previous test - line 911.

@Jomik
Copy link
Contributor

Jomik commented Jan 6, 2024

We are missing tests for this change, it is a bit hard to see what this actually accomplishes in the context of this PR.

I thought I did add the tests, but maybe I am missing something about the project structure? Thought dist/screeps-tests.ts contains tests, I added lines 919 - 921 with a test from the issue reported and fixed previous test - line 911.

You did, I just didn't have coffee yet 😅
Maybe add the same with an explicit false for the asArray argument.
Then this looks good. Amazed that this has been an error for so long .

@zyzyzyryxy
Copy link
Contributor Author

We are missing tests for this change, it is a bit hard to see what this actually accomplishes in the context of this PR.

I thought I did add the tests, but maybe I am missing something about the project structure? Thought dist/screeps-tests.ts contains tests, I added lines 919 - 921 with a test from the issue reported and fixed previous test - line 911.

You did, I just didn't have coffee yet 😅 Maybe add the same with an explicit false for the asArray argument. Then this looks good. Amazed that this has been an error for so long .

Done

Copy link
Member

@DiamondMofeng DiamondMofeng left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this issue

@DiamondMofeng DiamondMofeng merged commit 12cfd11 into screepers:master Jan 6, 2024
3 checks passed
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