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

test: add test for unnamed returns #22

Merged
merged 8 commits into from
Jan 26, 2024
Merged

test: add test for unnamed returns #22

merged 8 commits into from
Jan 26, 2024

Conversation

dristpunk
Copy link
Contributor

@dristpunk dristpunk commented Jan 23, 2024

🤖 Linear

Closes BES-248

ToDo:

  • [ ] Advanced parameters search algorithm (not linear)
  • Log index of missing unnamed returns

Copy link

linear bot commented Jan 23, 2024

BES-248 Unnamed returns

@dristpunk dristpunk marked this pull request as draft January 23, 2024 13:24
@dristpunk dristpunk self-assigned this Jan 23, 2024
@dristpunk dristpunk marked this pull request as ready for review January 24, 2024 15:01
expect(result).toEqual([`@return missing for unnamed return №2`]); // only 1 warning
});

it('should not warn of extra natspec tags', () => {
Copy link
Member

Choose a reason for hiding this comment

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

The code for this case won't compile, right? If so we can delete the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code for this case won't compile, right? If so we can delete the test.

right, and a couple of more tests

};

const result = validator.validate(node, natspec);
expect(result).toEqual(['@return _isMagic is missing']); // 1 warning
Copy link
Member

Choose a reason for hiding this comment

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

The warning is misleading here. As a user, I will check that the @return _isMagic is there and think that the tool has mixed something up. Some will realize it's because of the order but many won't. We should either make it the message or avoid printing any warnings in case of incorrect ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The warning is misleading here. As a user, I will check that the @return _isMagic is there and think that the tool has mixed something up. Some will realize it's because of the order but many won't. We should either make it the message or avoid printing any warnings in case of incorrect ordering.

this won't compile as well so I'll delete it

@@ -95,7 +95,7 @@ export function parseNodeNatspec(node: NodeToProcess): Natspec {
result.inheritdoc = { content: tagMatch[2] };
}
} else if (tagName === 'param' || tagName === 'return') {
const tagMatch = line.match(/^\s*@(\w+) *(\w+) (.*)$/);
const tagMatch = line.match(/^\s*@(\w+) *(\w*) *(.*)$/);
Copy link
Member

Choose a reason for hiding this comment

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

Good job catching this 👍🏻

/// fun fact: there are extra spaces after the 1st return
/// @return
/// @return
function functionUnnamedEmptyReturn() external view returns (uint256, bool){
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this function to ParserTestFunny? It's the contract that acts as a collection of weird natspec.

test/validator.test.ts Outdated Show resolved Hide resolved
@dristpunk dristpunk requested a review from gas1cent January 25, 2024 18:26
@gas1cent gas1cent merged commit cf9fe52 into main Jan 26, 2024
4 checks passed
@gas1cent gas1cent deleted the fix/unnamed-returns branch January 26, 2024 11:03
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.

2 participants