Minor tweaks in README, others #102
Conversation
| @@ -1,10 +1,11 @@ | |||
| # LCSC API | |||
|
|
|||
| - to get a product page based on LCSC number use GET request on | |||
| 1. To get a product page based on LCSC number, use GET request on | |||
There was a problem hiding this comment.
What's the reason for making this numeric list? There isn't a clear sequence "first this and then this". The bullets serve as a clear graphical division between two parts of the text.
There was a problem hiding this comment.
Numerical lists allow for discussion among teams better, which is important for software like this which may be prone to forking/external use. Either a short subheading or ordered list is most appropriate here, in my opinion.
Am happy to revert the list type change, if requested.
| The site and parts cache is hosted on GitHub Pages. | ||
|
|
There was a problem hiding this comment.
This comment makes no logical sense here. This is the first information the user (e.g., electrical engineer) sees when using this project. Why should they care about the internal implementation details of the project? This just clutters the text for them.
There was a problem hiding this comment.
We should factor the whole ## Developer section out then. As a developer trying to contribute to this project, I had an embarassingly tough time figuring out how to get started.
There was a problem hiding this comment.
I agree that having a separate dev section is a good direction.
README.md
Outdated
| Travis CI download XLS spreadsheet from the JLC PCB page, a Python script | ||
| process it and it generates per-category JSON file with components. | ||
| The page has no backend so it can be easily hosted on GitHub Pages. | ||
| Travis CI downloads the XLSX spreadsheet from the JLCPCB page, a Python script |
There was a problem hiding this comment.
When we're updating the docs, it is worth it to change Travis to GH Action we use.
There was a problem hiding this comment.
I agree. That's why I made the change. This is the only instance of Travis.
There was a problem hiding this comment.
I probably explained myself wrongly; the original wording is out-dated. We no longer use Travis CI, we use GH Actions. However, even after your update, you say we use Travis.
There was a problem hiding this comment.
Good catch, thank you. Fixed.
| $ virtualenv venv | ||
| $ source venv/bin/activate | ||
| $ pip install -e . | ||
| ``` | ||
|
|
||
| Then to download the cached parts list and process it, run: | ||
| Then to download the cached parts list (from the GH Pages site, generated via GH Actions) and process it, run: |
There was a problem hiding this comment.
Again, I find this extra information rather confusing - like I can see it is downloaded from the GH pages from the URL. And GH pages are in 99+% generated in GH actions.
There was a problem hiding this comment.
GH Pages are not in 99% of GH Actions. GH Actions generally run unit tests. I know that you think it's obvious. I'm an experienced dev, and I did not think it's obvious, hence why I spent my weekend updating these docs. If you think that it's that obvious that I'm the only person who could get confused on this, feel free to request that I revert this change.
There was a problem hiding this comment.
You misinterpreted my saying (and inverted the implication):
- I say "something is provided via GH Pages" implies "it was probably built using CI, most probably GH Actions"
- You read "We use GH Actions" implies "We have GH Pages"
What I dislike is "random pieces of information" scattered across the documentation. If you want to improve something, there should be "architecture overview" that captures the high-level design. It shouldn't pollute step-by-step instruction that the user is expected to just follow.
There was a problem hiding this comment.
Propose a solution. Tell me what you want. We agree on the problem.
There was a problem hiding this comment.
As I said in another thread - a new document describing the overall architecture. But building such a document is currently out of my time scope.
There was a problem hiding this comment.
If it's out of your timescope, can you just accept this change, and I'll split it out into a separate file later?
|
I've made the changes. Can you please check over. |
yaqwsx
force-pushed
the
master
branch
2 times, most recently
from
3dc9b5b
to
bb8f331
Compare
Fixes #100