-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Add path
field check in multi project repos
#215
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Were you able to test this? Can we try it out with https://github.com/kitspace-forks/DIY_particle_detector ?
Looks like build is reporting as failing because |
Does http://add-project-path-to-yaml.preview.kitspace.org/ still get updated? I did not see the repo I added on there. |
Yes, it should still be uploaded. Can't see the particle detector either though. Let's see when the latest build completes... |
Looking good! Do you think you could throw in the |
Wait, does the path do anything beside look for the readme at the moment? Are defaults |
It is used here in the Lines 131 to 166 in 31e073f
I also see I used this logic in a few places which I think we can get rid of, making use of the folder path from above. kitspace/tasks/page/processBOM.js Lines 16 to 22 in 2d92bfd
|
Testing with taking out the kitspace/tasks/page/processBOM.js Lines 45 to 49 in 2d92bfd
This section isn't working as intended with |
It finding the gerbers may just be because it scans the whole repo for suitable files. Is there a |
Yes, I moved and renamed the file in that case.
It seems the path isn't being built up correctly. I will look at it some
more tomorrow and better understand what's going on there.
…On Wed, Dec 4, 2019, 23:29 Kaspar Emanuel ***@***.***> wrote:
It finding the gerbers may just be because it scans the whole repo for
suitable files. Is there a .tsv file when it shows that error for the
bom? Our example has .csv files and they are in a bom/ folder.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#215?email_source=notifications&email_token=AFNJVHKFTC2SMG64WAKLOYLQXAVMRA5CNFSM4JUAVRJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEF6XBSA#issuecomment-561869000>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AFNJVHOZ7QFSW2QIXDN5HSLQXAVMRANCNFSM4JUAVRJQ>
.
|
Sorry to go back and forth on this but I think it actually makes much more sense to just be able to set a |
Got a bit busy before the holidays, sorry for disappearing on this. I agree the path field is confusing things, and I think they way I originally implemented it can be cleaned up. Do you think a new branch to add the |
Sure, no problem, happy holidays! Up to you if you want to use this branch for `readme` or if you want to start a new one.
…On 22 December 2019 13:31:30 GMT, Steven Bowen ***@***.***> wrote:
Got a bit busy before the holidays, sorry for disappearing on this.
I agree the path field is confusing things, and I think they way I
originally implemented it can be cleaned up.
Do you think a new branch to add the `readme` field is the way to go?
We can close this PR and perhaps I will revisit it once the `readme`
field is added?
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#215 (comment)
--
Sent from my mobile. Please excuse my brevity.
|
Closed in favor of #220 |
Closes #183 🎉