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 Online plugin #113

Closed
wants to merge 3 commits into from
Closed

Add Online plugin #113

wants to merge 3 commits into from

Conversation

bramtayl
Copy link

Updates #79 to the new infrastructure

@codecov
Copy link

codecov bot commented Nov 23, 2019

Codecov Report

Merging #113 into master will decrease coverage by 1.03%.
The diff coverage is 85.18%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #113      +/-   ##
=========================================
- Coverage   92.13%   91.1%   -1.04%     
=========================================
  Files          15      16       +1     
  Lines         318     371      +53     
=========================================
+ Hits          293     338      +45     
- Misses         25      33       +8
Impacted Files Coverage Δ
src/plugin.jl 90.9% <ø> (ø) ⬆️
src/PkgTemplates.jl 100% <ø> (ø) ⬆️
src/template.jl 100% <100%> (ø) ⬆️
src/plugins/online.jl 82.22% <82.22%> (ø)

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 39350c1...69036f5. Read the comment docs.

@nickrobinson251
Copy link
Collaborator

Is this tied to Github? If so I do not think it should have such a generic name as Online

@bramtayl
Copy link
Author

Yes it is. What should I call it?

@bramtayl
Copy link
Author

bramtayl commented Dec 9, 2019

Bump

@christopher-dG
Copy link
Member

Hi, I'll have a look sometime this week or weekend, I've been busy.

@bramtayl
Copy link
Author

bramtayl commented Dec 9, 2019

Np. The rewrite looks great!

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.

I started reviewing specifics but I realized that pointing out code style stuff is really silly of me, I'll just change that stuff myself. Here are some more high-level comments:

The name is fine but we can make this less specific to the GitHub+Travis+Documenter use case. Something along these lines:

put_online(t, p) = nothing
put_online(t, p::Git) = # Create Git{Hub,Lab,etc.} repo
put_online(t, p::Documenter{TravisCI}) = # Create DOCUMENTER_KEY
# And so on for other plugins that can have some online component. This is extensible by outside plugins too. 

hook(::Online, t::Template, ::AbstractString) = foreach(p -> put_online(t, p), t.plugins)

I'd prefer to keep the online settings stuff out of Template, and in Online.
Maybe something like:

struct Online fields... end
Online(settings::AbstractString) = # get the options from this file 
Online() = Online(some_default_path)

The default file could be managed by PkgTemplates (stored somewhere in the user's installation). with an interactive prompt to create it the first time.

I'd be happy to take on that extra work myself, since the groundwork is already here thanks to you. Maybe I could pull this into a separate branch and work from there for a while?

),
body = body
)
if response.status >= 300
Copy link
Member

Choose a reason for hiding this comment

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

This code never gets reached, the HTTP request itself will raise an exception.

if response.status >= 300
error("$(response.status) $(response.statustext): $(String(response.body))")
end
response.body
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
response.body
return response.body

@bramtayl
Copy link
Author

Sure, feel free to change it however you want

@bramtayl
Copy link
Author

So...I think github actions are amazing and basically make this ancient tech. Feel free to reopen if you want.

@bramtayl bramtayl closed this Feb 15, 2020
@bramtayl
Copy link
Author

Well, I've updated OnlinePackage to work with GitHub actions

@bramtayl
Copy link
Author

@christopher-dG it looks like documenter doesn't require a separate SSH key anymore, and neither does codecov, but CompatHelper does, so I think this is still useful. Do you want me to try to revive the PR?

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.

None yet

3 participants