Skip to content

Improve repository for newcomers#2380

Merged
Julow merged 29 commits intoocaml-ppx:mainfrom
tmattio:contributor-friendly
Sep 4, 2023
Merged

Improve repository for newcomers#2380
Julow merged 29 commits intoocaml-ppx:mainfrom
tmattio:contributor-friendly

Conversation

@tmattio
Copy link
Copy Markdown
Contributor

@tmattio tmattio commented Jun 24, 2023

Following on ocaml/odoc#962, this PR contains a few minor improvements and cosmetic changes to the repository to make it (hopefully) friendlier for newcomers.

Among the changes:

  • The addition of a ROADMAP.md with the projects that we have discussed in the context of ocamlformat 1.0
  • Improvements to the README
  • The addition to the authors and maintainers in the .opam files
  • Renaming and moving files for consistency with other projects.

The roadmap deserves special attention, let's be sure we agree on it before merging. Once its merged, I'll write a post on Discuss that links to it: users have been asking for clarity on the future of ocamlformat.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented Jun 24, 2023

Could this all be a linting check performed by a Github Action? Surely this is something that could benefit all OCaml projects so people might be interested in adding this check to the CI of their projects.

@tmattio
Copy link
Copy Markdown
Contributor Author

tmattio commented Jun 24, 2023

GitHub has https://github.com/ocaml-ppx/ocamlformat/community. I'm not sure what more can be done automatically. If someone is interested in building a linter for this, why not, but I see more value in focusing on the concrete changes (e.g. writing the roadmap isn't something that can be automated)

Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

Nice! The contributing guide was not uptodate so there's a few corrections to do.

Comment thread ocamlformat-bench.opam Outdated
Comment thread .ocp-indent
Comment thread CONTRIBUTING.md Outdated
5. Remain engaged with the issue until closure as your feedback might be needed.

When acknowledged, the project maintainers will add [labels](#ocamlformat-labels) to your issue throughout its lifespan.
Once your issue is acknowledged, our maintainers will apply relevant [labels](#ocamlformat-labels) to track its status.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Talking about the labels seems unnecessary. Perhaps it can be removed to be straight to the point ? (same in pull requests)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I've removed the mentions that maintainers use labels.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me

Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread ROADMAP.md Outdated

- **Versioning:** Ocamlformat 1.1 should be backward compatible and allow users to format code as they did with 1.0 if the version is specified in the configuration file.
- **Customization:** Continue to support and add formatting options ocamlformat users care about.
- **Concrete Syntax:** We'll migrate ocamlformat from an AST to a CST that contains location information to improve the formatting of comments.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That's not the reason, the current AST contains locations and we are already capable of adding the missing locations to it.

The CST will make the code more robust to bugs by removing the need to guess the formatting of an abstract syntax that often have several possible syntaxes.
It could help with comments in an unspecified way, but that will not be related to locations.

Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

I agree with @Julow's comments, I made some suggestions but otherwise looks good to me.

Comment thread ocamlformat.opam Outdated
Comment thread dune-project Outdated
Comment thread ROADMAP.md Outdated

- **Versioning:** Ocamlformat 1.1 should be backward compatible and allow users to format code as they did with 1.0 if the version is specified in the configuration file.
- **Customization:** Continue to support and add formatting options ocamlformat users care about.
- **Concrete Syntax:** We'll migrate ocamlformat from an AST to a CST that contains location information to improve the formatting of comments.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- **Concrete Syntax:** We'll migrate ocamlformat from an AST to a CST that contains location information to improve the formatting of comments.
- **Concrete Syntax:** We'll migrate ocamlformat from an AST to a CST that contains more location information and preserve the original syntax.

Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
Comment thread CONTRIBUTING.md Outdated
@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Aug 31, 2023

I applied all of my suggestion and rewrote partly the "Running the Tests" guidelines. It would be nice if you could review my changes.

Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

The ignore file in vendor/ do not seem to work as intended.
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

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

No need to wait more, I'll merge once the CI is green.

Comment thread .ocamlformat-ignore
@@ -1 +0,0 @@
vendor/**
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This file seems necessary, I'll add it back. The new one in vendor do not seem to work.

@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Sep 4, 2023

The lint-opam check is failing and lower-bound do not check the lowest version we pretend to support for cmdliner and csexp. Though, this is unrelated to this PR, which do not change the code.

@Julow Julow merged commit aa99c1d into ocaml-ppx:main Sep 4, 2023
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