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

Custom code style #1340

Open
krlmlr opened this issue Apr 9, 2024 · 14 comments
Open

Custom code style #1340

krlmlr opened this issue Apr 9, 2024 · 14 comments
Milestone

Comments

@krlmlr
Copy link
Contributor

krlmlr commented Apr 9, 2024

For graph literals.

#673 contains the logic that was used in a one-off run. It would be nice to have the code styled on a regular basis, e.g., in a pre-commit hook.

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

In the PR, there are two scripts defining the same function normalize_igraph_arrows().

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

I'd suggest:

Does that sound good @krlmlr?

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 3, 2024

Can that be a function in the igraph package too? (Not sure a whole new repo is justified here.)

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

I suppose it could, but I find it cleaner this way. https://github.com/igraph/igraph.r2cdocs is a separate package for instance.

If it is a function in the igraph package, could we export it? Styling graphs might be relevant for igraph users too.

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 3, 2024

Yeah, let's start with a separate package. Merging is easier than tearing apart.

We have infrastructure to run styler on every PR, need to understand how to make it work with that new package or style guide. No PR command workflow needed.

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

@krlmlr are all these snaps correct? https://github.com/igraph/igraph.style/blob/main/tests/testthat/_snaps/igraph-style.md

In the PR you had linked to, I think all versions of normalize_igraph_arrows() were the same but they contained leftover browser().

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 3, 2024

  • Needs a space before the RHS: A +-+ B etc.
  • Also: A -+ B:C

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

Right, so does it mean you had used other code in your PR? I'm confused.

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

oh maybe the functions aren't the same (I had tried to compare them with version control, maybe I pasted the wrong one 🫠 )

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

RHS problem fixed but did the styling of styler::style_text("A - +B:C", style = igraph_style) ever work?

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 3, 2024

Might not have worked ever, or styler changed in the meantime.

@maelle
Copy link
Contributor

maelle commented Dec 3, 2024

Do you still remember enough about the code to fix {igraph.style} yourself or should I try?

@krlmlr
Copy link
Contributor Author

krlmlr commented Dec 3, 2024

I don't have the capacity to deal with that at the moment.

@krlmlr
Copy link
Contributor Author

krlmlr commented Jan 9, 2025

Seems to work now. Next steps:

  • Run locally on the package, PR
  • Decide on a level of quality (strict, scope)
  • Configure GHA to run automatically

I can help with the third step.

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

No branches or pull requests

2 participants