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

Import changes from JuliaQuantumControl/QuantumCitations.jl #2

Closed
wants to merge 51 commits into from

Conversation

fredrikekre
Copy link
Member

Johann Antonio Duffek and others added 30 commits July 31, 2021 12:37
Refactor the code for rendering both citations and the bibliography such
that the rendered text is obtained from a `format_citation` and
`format_bibliography_entry` function, respectively. Moreover, in the
bibliography, the citation key is rendered via
`format_bibliography_key`.

These functions provide hooks for a user to completely customize the
rendering. For example, a user could decide to use the [`alpha` citation
style](https://www.bibtex.com/s/bibliography-style-base-alpha/) by
defining an appropriate `format_citation` and `format_bibliography_key`
function.
Citations are numbered in the order in which they appear (according to
the navigation)
At some point, the modifications to the .bib file should be reverted and
a better handling of math / protected strings in titles should be
implemented.
@goerz
Copy link
Member

goerz commented Jul 10, 2023

Ah, thanks!

That's exactly what I had in mind (and I was just about to do the same). I can add some more commits to this to further adapt documentation and CI as necessary. If we're keeping the numeric style as the default, it shouldn't be that much work.

Once we're happy with this branch, I would recommend merging it manually with --no-ff on the command line, so as to keep the branching visible in the history.

@fredrikekre
Copy link
Member Author

Yep, I won't add more commits until tomorrow, at least the last one does the rename (blind search and replace so haven't checked everything). I believe CI is mad because of the UUID now (should be changed back too).

Also, I hope it is okay to remove some of the JuliaQuantum infrastructure now that it is maintained under JuliaDocs? My computer grounded to a halt when precompiling QuantumTestUtils so would be happy to get rid if that, for example. But we can make these changes gradually in separate PRs.

Would be good to update the docs before merge though, so it doesn't say "DocumenterCitations is a fork if DocumenterCitations" etc.

@goerz goerz force-pushed the fe/JuliaQuantumControl/QuantumCitations.jl branch from f03aa99 to 8c3c4cd Compare July 11, 2023 02:31
Best not to depend on QuantumControlTestUtils
@goerz
Copy link
Member

goerz commented Jul 11, 2023

Yep, I won't add more commits until tomorrow, at least the last one does the rename (blind search and replace so haven't checked everything). I believe CI is mad because of the UUID now (should be changed back too).

Yes, I changed the UUID back to the original one. Together with some other fixes, the tests now run through.

Also, I hope it is okay to remove some of the JuliaQuantum infrastructure now that it is maintained under JuliaDocs? My computer grounded to a halt when precompiling QuantumTestUtils so would be happy to get rid if that, for example. But we can make these changes gradually in separate PRs.

Yes, totally. We should get rid of any QuantumControl org-specific tooling. I removed the dependency, but copied some of that tooling into the .jl files in ./tests. The _TestLogger is mostly there because I think Julia 1.6 doesn't have a TestLogger in the standard library. The coverage reporting in make test might be a little gratuitous, but it works for now. If we end up thinking that there's too much tooling or too many test dependencies there, we can simplify that in future PRs.

#1 (comment):

I like numeric as the default, but it is easy to customize anyway so it doesn't matter much. I would assume we would make a new release anyway, so I think it is fine to change the default if you want.

That definitely works for me, and should speed up the merge quite a bit. It's straightforward to customize for any user (just add a style keyword when instantiating CitationBibliography in docs/make.jl). I'd consider it a breaking change, but that should be fine. I suppose we'll be targeting a breaking release 0.3.0 of the forked DocumenterCitations.

Incidentally, I think I changed the minimum Julia version from 1.4 to 1.6 (since everything in the QuantumControl org has a minimum of 1.6), so that's also a breaking change. The CI test matrix only does Julia 1.6 on Linux right now. It would probably be good to extend that to Julia 1.9 and maybe also other platforms.

Would be good to update the docs before merge though, so it doesn't say "DocumenterCitations is a fork if DocumenterCitations" etc.

Definitely. Give me a few days to rewrite the README and documentation a bit to something more appropriate. I don't think we have to rush the merge, but it shouldn't take very long, either.

Do we need to do something to get the gh-pages deployment working? That might have to wait until the PR is merged into master, right? Also, coverage reporting. I don't know if it's a problem that the repo is officially a fork. Worst case, we'll have to delete the repo, create it again with the same name (so Github doesn't know it's a fork), and re-push all commits.

@goerz
Copy link
Member

goerz commented Jul 11, 2023

The CI test matrix only does Julia 1.6 on Linux right now. It would probably be good to extend that to Julia 1.9 and maybe also other platforms.

The failure on Windows seems more like a "syntax error" in ci.yml:

https://github.com/JuliaDocs/DocumenterCitations.jl/actions/runs/5515980612/jobs/10056866179?pr=2

Unfortunately, I haven't touched a Windows system in 20 years, so I'm not quite sure what exactly is wrong there. Any ideas?

@fredrikekre
Copy link
Member Author

Windows shell syntax is impossible to get right with quotes, but you can use julia as the shell instead, see https://discourse.julialang.org/t/tip-use-julia-as-custom-shell-in-github-actions/53377

@fredrikekre
Copy link
Member Author

Codecov seems activated, just don't have coverage for the master branch to compare with. I added an SSH deploy key for Documenter.

@fredrikekre fredrikekre marked this pull request as ready for review July 11, 2023 07:54
@fredrikekre fredrikekre deleted the fe/JuliaQuantumControl/QuantumCitations.jl branch July 11, 2023 08:53
@fredrikekre
Copy link
Member Author

Merged in 3b20824, docs still needs fixing, but thought it would be easier to do it afterwards after all.

@goerz
Copy link
Member

goerz commented Jul 11, 2023

Ok, sounds good! I'll start another PR on that…

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