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

Updated error message to indicate that modules must start with a letter #8253

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Updated error message to indicate that modules must start with a letter #8253

merged 1 commit into from
Aug 11, 2023

Conversation

hughesjs
Copy link
Contributor

This partially addresses #8252 by ensuring that the produced error message indicates that a module name must start with a letter.

Copy link
Collaborator

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

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

Looks like a reasonable improvement. Reminds me of e.g. 0install, which could run into this issue.

@Leonidas-from-XIV
Copy link
Collaborator

@hughesjs Can you promote the test case failures?

@hughesjs
Copy link
Contributor Author

hughesjs commented Aug 2, 2023

@Leonidas-from-XIV - Will give it a shot later this evening. No promises though, I've spent all of 2 hours writing ocaml and half of that was working out why I couldn't build the base project, I'll let you know how I get on

@Leonidas-from-XIV
Copy link
Collaborator

If you do dune runtest it will run the tests locally (and trigger the failures you see on CI) and you can call dune promote <path-of-test> to automatically promote the ones where the message changed to have the new message and commit those.

@Alizter
Copy link
Collaborator

Alizter commented Aug 2, 2023

You don't even have to run the entire test suite. Just do the following:

$ ./dune.exe build @github5273 @github7600 @pp @fmt
$ ./dune.exe promote

That just runs the tests that failed in CI and code formatting.
(also run make bootstrap if you haven't already done that).

@hughesjs
Copy link
Contributor Author

hughesjs commented Aug 2, 2023

Huh, didn't know that was a thing. Cheers! Will do it when I'm back at my own machine

@hughesjs
Copy link
Contributor Author

hughesjs commented Aug 2, 2023

Right, that's the test cases promoted

@Leonidas-from-XIV
Copy link
Collaborator

I think it is ready to be merged.

@Leonidas-from-XIV
Copy link
Collaborator

I've rebased the change upon our current main branch due to an annoying formatting change that sucks to resolve manually.

@Alizter
Copy link
Collaborator

Alizter commented Aug 10, 2023

@rgrinberg Can you take a look?

Signed-off-by: James Hughes <james@pyrosoftsolutions.co.uk>
Signed-off-by: Marek Kubica <marek@tarides.com>
@Leonidas-from-XIV Leonidas-from-XIV merged commit ad3b888 into ocaml:main Aug 11, 2023
@hughesjs hughesjs deleted the hughesjs-numeric-first-char branch September 7, 2023 16:17
@hughesjs
Copy link
Contributor Author

hughesjs commented Sep 7, 2023

Thanks for merging this guys, sorry I've not been particularly active here, life's being a bit hectic

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.

4 participants