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 filepath to main.ts in customizing_cli.md #271

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

mnikander
Copy link
Contributor

@mnikander mnikander commented Jan 20, 2025

The CLI section and generate of the minilogo tutorial reference a file 'src/cli/index.ts' which does not exist. Updated the path to 'src/cli/main.ts' instead. Make the path explicit, even when it is mentioned the second time, to avoid possible confusion with other 'main.ts' files.

@mnikander mnikander had a problem deploying to pull-request-preview January 20, 2025 08:06 — with GitHub Actions Failure
@mnikander
Copy link
Contributor Author

mnikander commented Jan 20, 2025

@msujew: I'm new to TypeScript, Langium, and to the etiquette regarding pull requests open-source projects (this would be my first contribution). I hope that this change is correct, and that it is OK for me to open a Pull Request like this. I am not sure if there are any conventions regarding branch naming, commit messages, or PR descriptions which I need to adhere to. Please feel free to give me feedback or make changes to this PR!

@mnikander
Copy link
Contributor Author

Do I need to register with the eclipse foundation to be able to merge changes?

@montymxb
Copy link
Contributor

Hi @mnikander , thanks for the contribution! With regards to etiquette, naming, and such this is perfectly fine, looks good to me, and in fact this was an outdated reference that needed to be freshened up; thanks for catching that!

The one remaining blocker is that you'll need to sign the Eclipse Contributor Agreement for this project, & double check that you using the same email as the one on your commit. Once that's all done you should be pretty much good to go here (pending an approval).

@mnikander
Copy link
Contributor Author

mnikander commented Jan 20, 2025

@montymxb: Thanks for the reply! I signed the Eclipse Contributer Agreement and refreshed that stage in the CI, so that's covered! I'll wait for feedback and hopefully an approval.

@mnikander
Copy link
Contributor Author

I did a git grep "index.ts" on the repo and got another hit in 'generate.md'. I fixed that as well and amended it to the commit.

@mnikander mnikander temporarily deployed to pull-request-preview January 20, 2025 11:59 — with GitHub Actions Inactive
Copy link

github-actions bot commented Jan 20, 2025

PR Preview Action v1.4.6
Preview removed because the pull request was closed.
2025-01-21 16:27 UTC

@mnikander
Copy link
Contributor Author

mnikander commented Jan 21, 2025

I rebased onto the current main

@mnikander
Copy link
Contributor Author

Hi @Lotes I saw that you were the most recent person to review a pull request in this repo. Can you review this PR for me? Is there anything I can improve here? :)

@Lotes
Copy link
Contributor

Lotes commented Jan 21, 2025

Thanks for your contribution.

I think the index.ts is still there! It is inside the Minilogo repository.
You are correct that this file is gone when you start a new Langium project.

Can you add a footnote (or similar) to all occurrences, pointing out that this path varies from the provided repository? WDYT about it?

@montymxb
Copy link
Contributor

@Lotes we could also rename the index.ts to align with the current generation state? That would save us having to note it everywhere else.

@Lotes
Copy link
Contributor

Lotes commented Jan 21, 2025

@montymxb @mnikander Then, this change is ok. Let's rename it in Minilogo as well.
Who wants to do it?

@mnikander
Copy link
Contributor Author

mnikander commented Jan 21, 2025

I'm just trying to do so: TypeFox/langium-minilogo#16

Copy link
Contributor

@Lotes Lotes left a comment

Choose a reason for hiding this comment

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

LGTM

@mnikander
Copy link
Contributor Author

mnikander commented Jan 21, 2025

@Lotes I don't have write access, so I can't merge. Feel free to merge!

@Lotes Lotes merged commit c6163dc into eclipse-langium:main Jan 21, 2025
5 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