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

Spelling #119

Merged
merged 41 commits into from
Nov 4, 2021
Merged

Spelling #119

merged 41 commits into from
Nov 4, 2021

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Nov 4, 2021

This PR corrects misspellings identified by the check-spelling action.

The misspellings have been reported at jsoref@9d44960#commitcomment-59407961

The action reports that the changes in this PR would make it happy: jsoref@44b0dee

Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.

Copy link
Contributor Author

@jsoref jsoref left a comment

Choose a reason for hiding this comment

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

Most fixes were suggested by Google Sheets, however I did select a couple by hand, and I've reviewed the full set of changes and am happy with them.

Analysis/src/RequireTracer.cpp Outdated Show resolved Hide resolved
Compiler/src/Compiler.cpp Show resolved Hide resolved
VM/src/lgc.cpp Show resolved Hide resolved
extern/doctest.h Outdated Show resolved Hide resolved
*
* All credits and commendations have to go to the authors of the
* following excellent libraries.
*
* - linenoise.h and linenose.c (https://github.com/antirez/linenoise)
* - linenoise.h and linenoise.c (https://github.com/antirez/linenoise)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is missing the comment which we forgot to add - this is cpp-linenoise, https://github.com/yhirose/cpp-linenoise. Please submit these fixes to upstream and remove them from this for now, we'd be happy to take the new upstream version as our version isn't modified iirc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fwiw, both upstream PRs have merged. This has to be one of the most responsive ecosystems I've seen in recent memory.

@@ -372,7 +372,7 @@ inline void SendSequence(LPCWSTR seq)
inline void InterpretEscSeq(void)
{
int i;
WORD attribut;
WORD attribute;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes projects intentionally misspell reserved words -- afaict this isn't one of those cases.

(I've left a displaName alone because it was such a typo, although I personally would prefer displayName_ which I expect would have the same effect.)

extern/linenoise.hpp Outdated Show resolved Hide resolved
@@ -56,7 +56,7 @@ end

These rules are simple to implement. In any Lua parser there is already a point where you have to disambiguate an identifier that starts an assignment statement (`foo = 5`) from an identifier that starts a function call (`foo(5)`). It's one of the few, if not the only, place in the Lua grammar where single token lookahead is not sufficient to parse Lua, because you could have `foo.bar(5)` or `foo.bar=5` or `foo.bar(5)[6] = 7`.

Because of this, we need to parse the entire left hand side of an assignment statement (primaryexpr in Lua's BNF) and then check if it was a function call; if not, we'd expect it to be an assignment statement.
Because of this, we need to parse the entire left hand side of an assignment statement (primaryexp in Lua's BNF) and then check if it was a function call; if not, we'd expect it to be an assignment statement.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I searched and am fairly certain this is a vaguely significant typo as it stands alone in this repository and also appears to differ from the Lua BNF that I could find.

@@ -359,7 +359,7 @@ TEST_CASE_FIXTURE(Fixture, "table_insert_correctly_infers_type_of_array_2_args_o
CHECK_EQ(typeChecker.stringType, requireType("s"));
}

TEST_CASE_FIXTURE(Fixture, "table_insert_corrrectly_infers_type_of_array_3_args_overload")
TEST_CASE_FIXTURE(Fixture, "table_insert_correctly_infers_type_of_array_3_args_overload")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to ignore tests if that's a preference, afaict fixing this is harmless (I've actively ignored things like error'string of words' -- which annoys most spell checkers -- in tests here, although I believe adding a space wouldn't hurt the tests and would make them more readable).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixing test case names is fine, FWIW for now we're using the convention of C identifiers for test names - we've used to use boost.test and migrated to doctest earlier this year, so just in case we need to migrate again to a framework that doesn't support string literals as test names we're sticking with identifiers for now.

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 other thing is stuff like this: https://github.com/Roblox/luau/blob/586bef6a4cffec80f7f2c7d4ed397aacc8fa9313/tests/conformance/gc.lua#L126

The spell checker I'm using would be much happier with:

-for n in pairs(a) do error'cannot be here' end
+for n in pairs(a) do error 'cannot be here' end

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be fine to change as well I think, syntactically they are the same and stylistically having a space seems better. Alternatively error("cannot be here") is cleaner and more idiomatic :)

@LoganDark
Copy link
Contributor

I legitimately cannot tell the difference between automated / manual comments here. This is literally perfect, I had no idea this existed.

@regginator
Copy link

you mad man

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thank you! I think all of this is correct except for the "available" I highlighted in a comment, and we should roll back cpp-linenoise and patch upstream + update our version with the current when upstream takes it instead.

@regginator
Copy link

regginator commented Nov 4, 2021

sanboxing
Screenshot_20211104-093004_Chrome

Copy link
Collaborator

@zeux zeux left a comment

Choose a reason for hiding this comment

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

Thanks!

@asajeffrey asajeffrey merged commit 278e848 into luau-lang:master Nov 4, 2021
@asajeffrey
Copy link
Collaborator

Thanks!

@regginator

This comment has been minimized.

@jsoref jsoref deleted the spelling branch November 4, 2021 15:28
@jsoref jsoref mentioned this pull request Nov 5, 2021
@jsoref jsoref mentioned this pull request Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants