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

crystal plugin: initial version #2598

Merged
merged 8 commits into from Jun 21, 2019

Conversation

Projects
None yet
4 participants
@bcardiff
Copy link
Contributor

commented Jun 13, 2019

Initial plugin for crystal applications.

@bcardiff bcardiff force-pushed the bcardiff:crystal-plugin branch from 04e07e8 to 5287ebe Jun 13, 2019

@sergiusens sergiusens force-pushed the bcardiff:crystal-plugin branch from 3154b18 to f827c87 Jun 16, 2019

sergiusens added some commits Jun 19, 2019

@codecov-io

This comment has been minimized.

Copy link

commented Jun 21, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@7527d0e). Click here to learn what that means.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2598   +/-   ##
=========================================
  Coverage          ?   89.09%           
=========================================
  Files             ?      207           
  Lines             ?    14091           
  Branches          ?     2129           
=========================================
  Hits              ?    12554           
  Misses            ?     1086           
  Partials          ?      451
Impacted Files Coverage Δ
snapcraft/plugins/crystal.py 87.5% <87.5%> (ø)

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 7527d0e...d338ad9. Read the comment docs.

@cmatsuoka
Copy link
Collaborator

left a comment

Most points I would raise in the original code were addressed in the subsequent commits. Perhaps we could use a slightly less trivial example in the spread tests? Other than that it looks fine.

@@ -0,0 +1 @@
puts "hello world"

This comment has been minimized.

Copy link
@cmatsuoka

cmatsuoka Jun 21, 2019

Collaborator

Could we use a slightly more complex test with a require, just to make sure modules are correctly loaded?

This comment has been minimized.

Copy link
@bcardiff

bcardiff Jun 21, 2019

Author Contributor

The prelude of the std-lib is implicitly required. Otherwise the puts usage will not even compile.

Using an external dependency will probably will not be wanted since it will require some external connections, but I could extend the example if wanted. Requiring the sqlite3 connection could be an example (that will also require some addition build_package)

Another alternative is to require something from the std-lib that is outside the prelude (like Complex) but that will add no value IMO. Since the prelude is found.

The other thing that can be required is relative files to the project. But I don't think this is something that could break.

Let me know what is preferred.

This comment has been minimized.

Copy link
@cmatsuoka

cmatsuoka Jun 21, 2019

Collaborator

Thanks, if just using puts already exercises this, I think it's enough.

@sergiusens sergiusens merged commit 1ad037e into snapcore:master Jun 21, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.