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

ISSUE-151 - Accept Readme Path and Section Name as parameters: #159

Conversation

jmcconnell26
Copy link
Contributor

  • Update args to accept --readme-path and section-name values
  • Add logic to allow custom section names and custom README files
  • Add integration test for existing readme cases
  • Create new snapshots for README test cases

Signed-off-by: joshmc josh-mcc@tiscali.co.uk

@anderejd
Copy link
Contributor

anderejd commented Dec 6, 2020

Should we dogfood this feature and use it for the README.md in this project as well?

@jmcconnell26
Copy link
Contributor Author

Should we dogfood this feature and use it for the README.md in this project as well?

Ohh, I like this idea! Let me pop it in now.

@jmcconnell26 jmcconnell26 force-pushed the ISSUE-151-AcceptReadmePathAndSectionName branch 2 times, most recently from 4310293 to 0827975 Compare December 6, 2020 15:27
@jmcconnell26
Copy link
Contributor Author

I had a look at this, and it looks like generating a Safety Report for cargo-geiger runs into the issue seen here #5
Shall we put this change on hold, and I can take a look into this issue next?

From a quick look, there seems to be two issues, one where how we handle files which are unused due to the feature flags for the dependency (like here, for serde_json-1.0.59/src/features_check/error.rs, a file which simply contains "serde_json requires that either std (default) or alloc feature is enabled"), and then an other where syn looks to fail to parse the file.

As a first step, would it be worth landing an update the geiger crate to use the latest version of syn?

Thanks,
Josh

@jmcconnell26
Copy link
Contributor Author

Ahh, this actually looks to have been a bug I introduced with the first set of changes to write to a README.
I've raised a PR to fix here #162
The files which fail to scan for cargo-geiger all look to be legitimate.

Thanks,
Josh

@anderejd
Copy link
Contributor

anderejd commented Dec 6, 2020

Ahh, this actually looks to have been a bug I introduced with the first set of changes to write to a README.
I've raised a PR to fix here #162

Nice! 👍

Waiting for a minor change in #162 before merging.

@jmcconnell26 jmcconnell26 force-pushed the ISSUE-151-AcceptReadmePathAndSectionName branch 3 times, most recently from 53ae6f6 to 7e14223 Compare December 7, 2020 20:05
@jmcconnell26
Copy link
Contributor Author

Hmm, I've added this, but I think it needs a bit more work to get it looking nicely.
Let me have a further look and see if I can update it to format the report better in the README.md

@jmcconnell26 jmcconnell26 force-pushed the ISSUE-151-AcceptReadmePathAndSectionName branch from 7e14223 to 9c68aad Compare December 7, 2020 20:25
@jmcconnell26
Copy link
Contributor Author

@anderejd, do you want to put this PR on hold, and let me land a separate one with a new Charset for GitHub markdown, with some better formatting for the report?

@anderejd
Copy link
Contributor

anderejd commented Dec 7, 2020

@anderejd, do you want to put this PR on hold, and let me land a separate one with a new Charset for GitHub markdown, with some better formatting for the report?

Separating it into two PRs sounds good!

* Update args to accept `--readme-path` and `section-name` values
* Add logic to allow custom section names and custom README files
* Add integration test for existing readme cases
* Create new snapshots for README test cases

Signed-off-by: joshmc <josh-mcc@tiscali.co.uk>
@jmcconnell26 jmcconnell26 force-pushed the ISSUE-151-AcceptReadmePathAndSectionName branch from 9c68aad to 8f3137f Compare December 8, 2020 18:54
@anderejd
Copy link
Contributor

anderejd commented Dec 9, 2020

Ready for merge?

@jmcconnell26
Copy link
Contributor Author

@anderejd, I've landed #163, which adds some nice formatting, and adds the nicely formatted report to the README, but I think this one is good to merge first!

Thanks,
Josh

@anderejd anderejd merged commit 91594c9 into geiger-rs:master Dec 11, 2020
jmcconnell26 pushed a commit to jmcconnell26/cargo-geiger that referenced this pull request Dec 13, 2020
…admePathAndSectionName

ISSUE-151 - Accept Readme Path and Section Name as parameters:
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