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

Add .gitattributes for GitHub Linguist #154

Merged
merged 2 commits into from
Aug 22, 2020
Merged

Add .gitattributes for GitHub Linguist #154

merged 2 commits into from
Aug 22, 2020

Conversation

formatc1702
Copy link
Collaborator

@formatc1702 formatc1702 commented Aug 15, 2020

This is an attempt to use GitHub Linguist to

  • avoid the examples/ and tutorial/ directories to be included in the project's language statistics
  • mark any WireViz output files (TSV, CSV, GV, HTML, SVG, PNG) as generated files

Unfortunately, it seems that the statistics are only updated when merged into master, so maybe someone with more experience can confirm that my implemenation will actually have the intended effect.

@@ -0,0 +1,11 @@
docs/* linguist-documentation
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Coming in #111

Copy link
Collaborator

Choose a reason for hiding this comment

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

I accidentally deleted this part of my review 10 hours ago, but I've tried to reconstruct it here:
We don't strictly need an override for the docs/ or example/ directories as they seem to be covered by documentation.yml already, but I agree to keep them anyway to state the fact explicitly. I tested by renaming tutorial/ to docs/:
renaming

.gitattributes Outdated
Comment on lines 3 to 4
examples/* -linguist-detectable
tutorial/* -linguist-detectable
Copy link
Collaborator Author

@formatc1702 formatc1702 Aug 15, 2020

Choose a reason for hiding this comment

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

Does it make sene to move examples/ and tutorial/ into the docs/ directory?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A tutorial and examples are definitely important parts of the documentation, but moving them into the docs/ directory or not is perhaps a matter of taste. Some might argue that it might be less easy to discover them if moving them, and others might argue that they always start with the docs/ anyway and don't see a problem. If moving them, please include links to them both from the root README.md and from the docs/README.md.

We don't strictly need an override for the tutorial/ directory either, probably because the of the **/*.html linguist-generated further down in this file, but I agree to keep them anyway to state the fact explicitly. However, I suggest using linguist-documentation in both of them. I tested without any directory overrides:
nofolder

@formatc1702 formatc1702 added this to the v0.3 or later milestone Aug 17, 2020
Copy link
Collaborator

@kvid kvid left a comment

Choose a reason for hiding this comment

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

This is an attempt to use GitHub Linguist to

  • avoid the examples/ and tutorial/ directories to be included in the project's language statistics
  • mark any WireViz output files (TSV, CSV, GV, HTML, SVG, PNG) as generated files

Unfortunately, it seems that the statistics are only updated when merged into master, so maybe someone with more experience can confirm that my implemenation will actually have the intended effect.

I didn't have any experience with the language statistics either, so I did some tests in my forked repo:

  1. First, I had to force-push your master into my fork master to have the same starting point as you. (I originally forked from aakatz3 which had a master not in sync with your master.)
  2. Then I cherry-picked your single commit in this PR into my master and pushed that to my fork:
git cherry-pick aedd53393ca2052f2b8dc5f87d54b2a0ef510a94
git push kvid

Before the cherry-pick, the language statistics looked identical to yours:
before
After pushing the cherry-picked commit, the language statistics changed:
after

If this was the result you aimed for, then I conclude that you should consider including this PR in v0.2. I only have the very minor change suggestion about replacing -linguist-detectable with linguist-documentation to state that it is documentation, but it's not very important to me because the effect is the same.

.gitattributes Outdated
Comment on lines 3 to 4
examples/* -linguist-detectable
tutorial/* -linguist-detectable
Copy link
Collaborator

Choose a reason for hiding this comment

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

A tutorial and examples are definitely important parts of the documentation, but moving them into the docs/ directory or not is perhaps a matter of taste. Some might argue that it might be less easy to discover them if moving them, and others might argue that they always start with the docs/ anyway and don't see a problem. If moving them, please include links to them both from the root README.md and from the docs/README.md.

We don't strictly need an override for the tutorial/ directory either, probably because the of the **/*.html linguist-generated further down in this file, but I agree to keep them anyway to state the fact explicitly. However, I suggest using linguist-documentation in both of them. I tested without any directory overrides:
nofolder

@formatc1702
Copy link
Collaborator Author

Thank you very much for running this test. I've changed -linguist-detectable to linguist-documentation as suggested.
Moving examples and tutorial into docs/ can happen later. :)

@formatc1702 formatc1702 merged commit df90d83 into dev Aug 22, 2020
@formatc1702 formatc1702 deleted the feature/linguist branch August 22, 2020 16:49
@formatc1702 formatc1702 modified the milestones: v0.3, v0.2 Oct 17, 2020
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.

2 participants