JuliaCollections / OrderedCollections.jl

Julia implementation of associative containers that preserve insertion order
MIT License
92 stars 38 forks source link

OrderedDict missing `last` #91

Closed jmigual closed 1 year ago

jmigual commented 2 years ago

OrderedDict provides a first method but does not provide a last method:

julia> a = OrderedDict(1 => "a", 2 => "b")
OrderedDict{Int64, String} with 2 entries:
  1 => "a"
  2 => "b"

julia> first(a)
1 => "a"

julia> last(a)
ERROR: MethodError: no method matching lastindex(::OrderedDict{Int64, String})
Closest candidates are:
  lastindex(::Any, ::Any) at ~\scoop\apps\julia\current\share\julia\base\abstractarray.jl:373
  lastindex(::Union{SortedDict, SortedMultiDict, SortedSet}) at ~\.julia\packages\DataStructures\izXYB\src\tokens2.jl:19
  lastindex(::Number) at ~\scoop\apps\julia\current\share\julia\base\number.jl:90
  ...
Stacktrace:
 [1] last(a::OrderedDict{Int64, String})
   @ Base .\abstractarray.jl:473
 [2] top-level scope
   @ REPL[26]:1
pri1311 commented 2 years ago

Hi, I'm fairly new to Julia and was trying to navigate the codebase, to find the code linked to the issue. While scouting through different repositories I realized none(atleast from what I checked) don't support last function. Is there a base repo/code base for dict functions you could point me to?

StephenVavasis commented 2 years ago

Nobody answered you yet, so I will try to remember the steps for making a pull request from my last round of doing so, and maybe this will help.

  1. Log into your github account.
  2. Visit the DataStructures.jl page here: https://github.com/JuliaCollections/DataStructures.jl/
  3. Create a fork to your own github account using the 'fork' button (upper right).
  4. Create a local repository on your desktop using and then clone the fork from your github account. This requires some 'git' commands at the command prompt on your desktop.
  5. Using another git command, create a new branch with a descriptive name, e.g., 'fix_missing_last' and switch to that branch.
  6. Start Julia. Go to the package manager by typing ']'. Then type 'dev local_path_to_repository' to install the package in Julia in development mode.
  7. Edit the package until you are satisfied with the changes.
  8. Update the test set to test your modification.
  9. Update the documentation to reflect your modification.
  10. Use git commit as necessary to make commits to your local repository. However, ideally, you execute just one git commit command when everything is ready so that there is a clean history of commits.
  11. Finally, when the last git commit is executed, create a pull request with the git push command.

Here is an old post on discourse that might be helpful: https://discourse.julialang.org/t/how-to-contribute-to-existing-package/32749

jmigual commented 2 years ago

Thanks I'll see if I can look at this. I'm fairly new to julia so we'll see.

pri1311 commented 2 years ago

Hi @StephenVavasis, thank you for your detailed explanation. Although I really appreciate the effort, my question was more issue related, meaning I couldn't find the code linked to the particular issue. If you could help me further, it would be great!

StephenVavasis commented 2 years ago

Sorry, I misunderstood the question. The relevant source code is in ordered_dict.jl, which is not actually in this repository. It is in the adjacent repository: https://github.com/JuliaCollections/OrderedCollections.jl So this issue should be closed and reopened over there.

pri1311 commented 2 years ago

I checked the OrderedCollections repository already, I don't think the code for first and last functions lies in that repository. Also, even PropertyDicts doesn't have last function but supports first. It seems like all the specific dicts repos/packages have a default dict code someplace else.

oxinabox commented 2 years ago

I have moved the issue.

The reason it doesn't need to implement first is because it implements iterate anything that implements iterate gets first for free as it can be define efficiently as just iterating once -- there is a fallback definition. This is not the case for last.

I believe for OrderedDict there is a efficient and sensible way to define last. So a PR can and should be made here to add it

pri1311 commented 2 years ago

I tried working on this, but for some reason, the changes don't reflect. Not just my changes, but the previous commit adding popfirst also doesn't work on my PC.

Screenshot 2022-09-25 at 1 25 01 PM

Any idea what I'm doing wrong? Also, this is what I get after installing it again after making changes:

Screenshot 2022-09-25 at 1 27 57 PM
StephenVavasis commented 2 years ago

Is it possible you are forgetting to type using OrderedCollections when you restart the REPL? Also, please note the "revise" workflow:

https://timholy.github.io/Revise.jl/stable/

which is popular for package developers.