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 CITATION.bib #73

Merged
merged 10 commits into from
May 15, 2019
Merged

Add CITATION.bib #73

merged 10 commits into from
May 15, 2019

Conversation

matbesancon
Copy link
Contributor

Since the Julia community is starting to agree on a standardized CITATION.bib file, it would be great to be able to have it from PkkTemplates

@christopher-dG
Copy link
Member

I would advise adding this as a plugin instead of some special case. The package is in a bit of a weird state too, where the internals are completely redone in #61... I need to finish that to make it easier to contribute.

@matbesancon
Copy link
Contributor Author

so #61 will be blocking to switch to the plugin mechanism right? From the doc I wasn't sure this was supposed to be a plugin:

Generic plugins are plugins that add any number of patterns to the generated package's .gitignore

Since this was not related to .gitignore, I was doubting on it being a plugin

@christopher-dG
Copy link
Member

Oh I suppose the docs could be clearer, a GenericPlugin is basically something that has the capability to create a single file in the package (i.e. CITATION.bib) and/or add entries to the gitignore. Thanks for bringing that up.

The PR is not required to make this a plugin, it's just that the plugin would be rewritten to be one line with the new internals. If you'd like to get this merged before then, feel free to implement as a Citation plugin (you can basically copy the travisci.jl code to suit your needs).

Again, sorry contributing is a bit messy at the moment 🙃.

@matbesancon
Copy link
Contributor Author

this is indeed hairy, I think I managed to get something, still need to test it a bit

Copy link
Member

@christopher-dG christopher-dG 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 overall to me 🙂. Also please export the new plugin.

src/plugins/citation.jl Outdated Show resolved Hide resolved
src/generate.jl Outdated Show resolved Hide resolved
src/generate.jl Outdated
@@ -63,6 +63,7 @@ function generate(
gen_require(pkg_dir, t),
gen_readme(pkg_dir, t),
gen_license(pkg_dir, t),
gen_citation(pkg_dir, t),
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be deleted now.

@codecov
Copy link

codecov bot commented May 14, 2019

Codecov Report

Merging #73 into master will decrease coverage by 0.21%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #73      +/-   ##
==========================================
- Coverage   97.84%   97.63%   -0.22%     
==========================================
  Files          13       14       +1     
  Lines         372      381       +9     
==========================================
+ Hits          364      372       +8     
- Misses          8        9       +1
Impacted Files Coverage Δ
src/PkgTemplates.jl 100% <ø> (ø) ⬆️
src/generate.jl 98.18% <100%> (+0.03%) ⬆️
src/plugins/citation.jl 83.33% <83.33%> (ø)
src/template.jl 99.01% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 901a0a3...19a972b. Read the comment docs.

src/generate.jl Outdated Show resolved Hide resolved
src/template.jl Outdated Show resolved Hide resolved
src/plugins/citation.jl Outdated Show resolved Hide resolved
function gen_plugin(p::Citation, t::Template, pkg_name::AbstractString)
pkg_dir = joinpath(t.dir, pkg_name)
text = """@misc{$pkg_name.jl,
\tauthor = {{$(t.author)}},\n
Copy link
Member

Choose a reason for hiding this comment

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

Are there supposed to be two curlies like {{this}} or just {one}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Two maintains the uppercase but your right it'n not necessary for authors

pkg_dir = joinpath(t.dir, pkg_name)
text = """@misc{$pkg_name.jl,
\tauthor = {{$(t.author)}},\n
\ttitle = {{$(pkg_name).jl}},\n
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
\ttitle = {{$(pkg_name).jl}},\n
\ttitle = {{$pkg_name.jl}},\n

Also, same question as author.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I find it easier on the eyes to have explicit parentheses when the variable name is long-ish, otherwise it can lead to ambiguities on what belongs to the interpolated expression: $(pkg_name).jl or $(pkg_name.jl)

src/plugins/citation.jl Outdated Show resolved Hide resolved
end

@testset "File generation" begin
# Without a coverage plugin in the template, there should be no post-test step.
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it might be a copy-paste remnant

test/plugins/citation.jl Outdated Show resolved Hide resolved
@matbesancon
Copy link
Contributor Author

alright tests added and passing

@matbesancon matbesancon changed the title [WIP] add CITATION.bib Add CITATION.bib May 14, 2019
@matbesancon
Copy link
Contributor Author

@christopher-dG looks good on my side

Copy link
Member

@christopher-dG christopher-dG left a comment

Choose a reason for hiding this comment

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

There are a couple hanging comments still, but this looks good 🙂

src/generate.jl Outdated Show resolved Hide resolved
@christopher-dG
Copy link
Member

Awesome, thanks 🙂

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