-
Notifications
You must be signed in to change notification settings - Fork 6
Add Project.toml, speedup of isregistered and getkey, path consistency #14
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
Conversation
|
It's been a minute since you made this PR (sorry!). Are you still interested in having it merged? I don't understand why this change is being made. Why would we prefer |
|
It took some time for me to remember 😉
I think the most important about this PR in the end is the usage of So why not making the following change? |
|
@twavv I think this PR is still valid! I've brushed up the PR to be compatible with the latest JSON v1 and the test now also pass with Julia 1.12 |
|
I have no idea, sorry. |
|
@hhaensel hey I messaged you on Slack! I'm not sure if I can just hit merge here, yet, but willing to verify this with you. |
Project.toml
Outdated
| @@ -0,0 +1,20 @@ | |||
| name = "AssetRegistry" | |||
| uuid = "bf4720bc-e11a-5d0c-854e-bdca1663c893" | |||
| version = "0.1.0" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should definitely be a higher version because v0.1.0 is currently released. It should be v0.2.0, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As it is non-breaking it could as well be 0.1.1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my projects I tend to make a separate commit for the version change as that is better visible in the git tree
|
Added a changelog and set version 0.1.1. EDIT: in that case I'd remove the last commit and add that after including #16 |
Project.toml
Outdated
| [compat] | ||
| JSON = "0.0.0 - 0.21, 1" | ||
| Pidfile = "1" | ||
| julia = "1" No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, updating the compat of Julia means it's a breaking release, so usually the version bump should reflect that. 0.1.1 means that we cannot patch 0.1 for 0.7 only. Which I think is ok but just trying to use the semantic thing here because sometimes the reviewers will complain.
|
I think, we could have kept 0.1.1, or kept compat with julia 0.7. Anyhow the usage of 0.7 is not encouraged, so that wouldn't hurt anyone. |
During my work at JuliaGizmos/WebIO.jl#417, I realised some (minor) performance issues with
getkey(), which I thought was due to the recalculation of the key.Only later I realised that the main bottle neck - at least on my windows machine - is from
abspath(). Nevertheless, I propose a PR here to change getkey such that it checks for the values of the registry instead of the keys, and at the same time bring AssetRegistry to the new package format.