feature/dungeon #4
Conversation
|
Similar to the coin branch, maybe we should be targeting the merge into develop instead of master. |
|
merge has been retargeted |
|
I see that the deps were updated, but i don't see that lock file was committed, I think that file should be committed. |
Cargo.lock is typically only committed for binary packages, not library packages like this one. This is the behavior of the .gitignore generated by |
|
Hmm, not having the lock file for development purpose sounds like it can create issues where rebuilding the library with dependencies will cause changes in the behavior of the library that will not be reproducible and thus affect development. I can understand not using the .lock file in dep resolution for the end application. That should be on cargo do ignore the .lock file during the application dep resolution. Thus it sounds like we can't create deterministic builds for our library during development? according to this issue rust-lang/cargo#8728, it might be the case that the cargo.lock file does not affect downstream application dep resolution, which is inline with the link you posted, if i understood it correctly. As such i think its worth it to place the .lock in the repo for our own development and build reproduceability, and in alignment with that, (like done in other repos) there is a build CI job for updating the deps (i.e ignore the .lock file) and perform a build and test, that will then create a new PR that has the updated deps. This job is made with the goal to ensure that semvar updating of the deps works approprate from the stand point of that our tool works with the deps and that someone upstream didn't push api breaking changes into the crates repo. However an end user might notice this first, hence the reason for the CI to run fairly regularly so we'd notice first. |
|
The lock file discussion is not particularly relevant to this PR. We can open an issue or add it later. Are there other people or writings about commiting a lock file for libraries? To me both make sense, but I am leaning towards not commiting. This way when I pull a new lock file will not completely destroy my incremental builds. Also I like that the PR builds will be always using what a new executable project will be using, to hopefully catch any breaking changes in our deps quickly. Since I'm pretty indifferent on it I want to either use the rust default of non-commiting or not block this PR with this. |
okay just realized i can for some reason edit your reply,..... can you? The poetry package manager recommends committing the lock file for development purposes, mostly the reasons i tried to outline above, along with the links i provided. I think the issue for incremental things, hmm, I don't think i share the same level of concern on the increment build issue, doesn't cargo do incremental builds of the libraries? not all the deps may need to be rebuilt? Does it not store a version of each dep that it builds? this might make the .cargo folder kinda bloated with time however. I'm still in favor of commiting it as i see the pipeline having the purpose of both checking with the current deps and proposing PRs for the deps. I think github already has a PR dependabot for rust, i clicked enabled to see if it works, but i couldn't tell it worked by reading the lock file or by looking at the cargo.toml |
|
cloing with comment does not merge, sad |
Adding the dungeon module to provide information about dungeon rewards.
dungeon::Dungeon and dungeon::Path list all dungeons and their paths and provides information about their rewards.
dungeon::UserProgress is created from an API key and can be used to test if running a dungeon path will give bonus rewards thanks to the daily bonus and/or the Dungeon Frequenter achievement.