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 per-file project-loading #109

Merged
merged 2 commits into from
Nov 6, 2023
Merged

Conversation

nberth
Copy link
Collaborator

@nberth nberth commented Nov 2, 2023

Includes fixes to project directory lookup for indent sub-commands, as well as some basic renaming of modules to have a meaningful Superbol_free_lib.Project module.

@nberth nberth force-pushed the fix-project-loading branch from ece4162 to 52eb68f Compare November 2, 2023 18:12
@nberth nberth marked this pull request as ready for review November 3, 2023 09:49
@nberth nberth added the ok to review As its name says label Nov 3, 2023
@nberth nberth force-pushed the fix-project-loading branch 4 times, most recently from d43bb6b to 37ad445 Compare November 3, 2023 15:26
@nberth nberth requested a review from bclement-ocp November 6, 2023 08:39
@nberth nberth force-pushed the fix-project-loading branch from 37ad445 to d091b8a Compare November 6, 2023 08:51
Copy link
Contributor

@bclement-ocp bclement-ocp left a comment

Choose a reason for hiding this comment

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

The files vSCode_extension.ml{,i} should probably be called vscode_extension.ml{,i} instead. vSCode_extension.ml{,i} is really ugly; VSCode_extension.ml{,i} would be fine, but there are other references to Vscode using this case (e.g. Vscode_json, Vscode_languageclient) and consistency matters.

@nberth
Copy link
Collaborator Author

nberth commented Nov 6, 2023

The files vSCode_extension.ml{,i} should probably be called vscode_extension.ml{,i} instead. vSCode_extension.ml{,i} is really ugly; VSCode_extension.ml{,i} would be fine, but there are other references to Vscode using this case (e.g. Vscode_json, Vscode_languageclient) and consistency matters.

Yes, I am planning to rename all of those other ulgy file names. The name of the module matters (and should be VSCode`), not the name of the file.

@nberth
Copy link
Collaborator Author

nberth commented Nov 6, 2023

I can change to VSCode_extension.ml{,i} if you prefer.

@bclement-ocp
Copy link
Contributor

We will disagree here because I think that using Vscode rather than VSCode as the module name is also the most consistent with the rest of the project using snake_case, including for constructor names (not having to worry about whether a capital should be added in a module name or not is nice), but I am fine with any solution that doesn't involve vSCode (which is introduced in the PR, if I'm not mistaken).

@nberth
Copy link
Collaborator Author

nberth commented Nov 6, 2023

Hum, I always though VSCode was written like this (but I don't use it myself so I never checked).
Actually it seems "VS Code" (with a space) is the way it is spelled, which indicates VS_code_extension should be the module name (acronyms should stay in capitals even in module names). But as you point out we already have a name for that in the project so I'll keep Vscode_.

(At this point it should be clear that aesthetics, which is very subjective, also matter in choosing module names ;-) — I was much less comfortable with having VS_code_ than Vscode_ everywhere.)

@nberth nberth force-pushed the fix-project-loading branch from e4235ea to 23ba10e Compare November 6, 2023 11:06
@nberth nberth merged commit 1f41278 into OCamlPro:master Nov 6, 2023
3 checks passed
@nberth nberth deleted the fix-project-loading branch November 6, 2023 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok to review As its name says
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants