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

Added installation instructions for new pkg mode in README #20

Merged
merged 4 commits into from
Sep 16, 2018

Conversation

rahulkp220
Copy link
Member

@rahulkp220 rahulkp220 commented Sep 16, 2018

How to install/update using the pkg mode/prompt.

julia>] # switch to pkg> mode
(v1.0) pkg> 

@rahulkp220 rahulkp220 self-assigned this Sep 16, 2018
Copy link
Member

@carstenbauer carstenbauer left a comment

Choose a reason for hiding this comment

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

I guess it's a bit inconsistent in it's current form. To use Pkg.add I have to using Pkg. So the upper suggestions only works on 0.6 and the lower one only on >= 0.7.

In my opinion, we should only write it for the 1.0 world. This is the major audience and people from pre 1.0 time are likely to know how do add the package anyway. Concretely, I'd just add a using Pkg.

Personally, I'd also drop the whole "Update" section. It seems irrelevant to me.

README.md Outdated
@@ -13,19 +13,27 @@ A very simple package for accessing elements in the Periodic Table! :fire:

## Installation
Since PeriodicTable is registered in `METADATA.jl`, you can directly install it like,
```jl
```julia
julia> Pkg.add("PeriodicTable")
Copy link
Member

Choose a reason for hiding this comment

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

Add using Pkg here.

Copy link
Member Author

@rahulkp220 rahulkp220 Sep 16, 2018

Choose a reason for hiding this comment

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

Would this seem better? Later when we drop support for version 0.6, we will have this part removed.

# for version <= 0.6
julia> using Pkg
julia> Pkg.add("PeriodicTable")

# for version >= 0.7
julia>] # switch to pkg> mode
(v1.0) pkg> add PeriodicTable

Copy link
Member

Choose a reason for hiding this comment

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

No, the upper one won't work on 0.6 (there exists no module Pkg on 0.6). As I said, I'd just drop the # for version ... comments and only give instructions for 0.7+.

If you would really want to indicate both ways (0.6 and 0.7+) then I'd switch the order (0.7+ first) and remove the using Pkg.

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed that part.

README.md Outdated

# OR
julia>] # switch to pkg> mode
(v1.0) pkg> update PeriodicTable
Copy link
Member

Choose a reason for hiding this comment

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

I'd drop this section.

@carstenbauer
Copy link
Member

Slightly OT, but I also thought about reversing the order in the How it works section. Wouldn't it be better to have the full PT first and then explain how to access detailed element data?

One more thing (sorry :D), the "Data by" section should probably get a sentence that only parts of the data come from the JSON source. Some information has (and will be) added.

@rahulkp220
Copy link
Member Author

I guess the update section was always a carryover from the time this package was created, and I agree, we can have it removed as for someone who knows how to add a package, it would be too naive not to know how to update.

README.md Outdated
@@ -13,19 +13,27 @@ A very simple package for accessing elements in the Periodic Table! :fire:

## Installation
Since PeriodicTable is registered in `METADATA.jl`, you can directly install it like,
```jl
```julia
julia> Pkg.add("PeriodicTable")
Copy link
Member

Choose a reason for hiding this comment

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

No, the upper one won't work on 0.6 (there exists no module Pkg on 0.6). As I said, I'd just drop the # for version ... comments and only give instructions for 0.7+.

If you would really want to indicate both ways (0.6 and 0.7+) then I'd switch the order (0.7+ first) and remove the using Pkg.

README.md Outdated
* [Periodic-Table-JSON](https://github.com/Bowserinator/Periodic-Table-JSON)
### Data by
The data used for this package has been pulled up in parts from [here](https://github.com/Bowserinator/Periodic-Table-JSON)
Some information has been (and will be) added over the period of time.
Copy link
Member

Choose a reason for hiding this comment

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

"Some information has been (and will be) added over time." sounds better to me.

Also there seems to be a dot missing after the here link.

README.md Outdated
```
exports a global variable called `elements`, which is a collection of

In particular `PeriodicTable` exports a global variable called `elements`, which is a collection of
`Element` data structures. You can look up elements by name (case-insensitive)
Copy link
Member

Choose a reason for hiding this comment

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

I'd move the first sentence, "In particular PeriodicTable exports a global variable called elements, which is a collection of Element data structures. ", before the PT visualization.

@rahulkp220
Copy link
Member Author

Hopefully should be enough for now.

Since PeriodicTable is registered in `METADATA.jl`, you can directly install it like,
```jl
```julia
Copy link
Member

Choose a reason for hiding this comment

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

In the future we might have to replace METADATA.jl by the general registry on https://github.com/JuliaRegistries/General.

@carstenbauer carstenbauer merged commit 9045ffe into master Sep 16, 2018
@rahulkp220 rahulkp220 deleted the rahul-update-readme branch September 16, 2018 08:26
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