mre / mos6502

MOS 6502 emulator written in Rust
BSD 3-Clause "New" or "Revised" License
79 stars 15 forks source link

Starting on implementing different variants #88

Closed omarandlorraine closed 1 year ago

omarandlorraine commented 1 year ago

From what I can see, the failing CI/CD process has to do with linting skeptic, a build time dependency. How then shall I proceed?

mre commented 1 year ago

Skeptic checks that the examples in the README.md are up-to-date. If these get fixed, then it should work again.

omarandlorraine commented 1 year ago

Skeptic checks that the examples in the README.md are up-to-date. If these get fixed, then it should work again.

Can you read the output of the CI / lint (pull request)? The error is not coming from the README.

error: unnecessary hashes around raw string literal
  --> /home/runner/work/mos6502/mos6502/target/debug/build/mos6502-5d795f23ad8c7099/out/skeptic-tests.rs:53:10
   |
53 |   {}"####, r####"use mos6502::memory::Bus;
   |  __________^
54 | | use mos6502::memory::Memory;
55 | | use mos6502::instruction::Nmos6502;
56 | | use mos6502::cpu;
...  |
83 | | }
84 | | "####);
   | |_____^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes
   = note: `-D clippy::needless-raw-string-hashes` implied by `-D warnings`
omarandlorraine commented 1 year ago

Okay, I've sussed it.

Clippy is being run with --all-targets, which gets it to check all targets. But skeptic is unmmaintained, so it's beginning to fall behind. I suggest we remove either skeptic or --all-targets.

Which do you prefer?

omarandlorraine commented 1 year ago

I have disabled --all-targets for the time being, the tests are passing now

mre commented 1 year ago

Thanks. We can remove skeptic then. I hope there's a replacement of some sort.

omarandlorraine commented 1 year ago

We can remove skeptic then. I hope there's a replacement of some sort.

Do you want to find one? Or shall we remove the example(s) from the README and refer readers to the examples/ folder?

omarandlorraine commented 1 year ago

Thanks. We can remove skeptic then. I hope there's a replacement of some sort.

There is a replacement; cargo doctest can do it now. I'll open another pull request to bring the compile times down by removing skeptic and num, which should close #89 as well.

mre commented 1 year ago

Perfect. Thanks!

omarandlorraine commented 1 year ago

Perfect. Thanks!

You ready to merge the variety branch then, or? Still want to do the review?

mre commented 1 year ago

Oh, sorry, I was referring to your comment about removing skeptic and nom. 😅 Still have to do the review of this PR.

omarandlorraine commented 1 year ago

We should revert c87975e and then rebase this pull-request after #93 is in

mre commented 1 year ago

We should revert c87975e and then rebase this pull-request after https://github.com/mre/mos6502/pull/93 is in

It's in now. Feel free to revert. 👍

omarandlorraine commented 1 year ago

It's in now. Feel free to revert. 👍

That commit has been reverted and the PR has been rebased; so this is ready for merging as far as I am concerned

mre commented 1 year ago

Looks good. Just some minor comments. We can merge if you like as there are no blockers. Up to you. 😄

omarandlorraine commented 1 year ago

Great, are you happy to merge and do a release on crates.io?

mre commented 1 year ago

Sure!

mre commented 1 year ago

Done: https://crates.io/crates/mos6502/versions