search

srobo / tools

Student Robotics development helper scripts
4 stars 3 forks source link

Ignore README.md files within plain directories #24

Open PeterJCLaw opened 5 years ago

PeterJCLaw commented 5 years ago

But only within plain directories. This avoids potential confusion between whether to look in an 'info' file or a 'README.md' file by ensuring that only one of them is valid in a given location.

Fixes https://github.com/srobo/tools/issues/17.

This is based on #22; see https://github.com/srobo/tools/commit/5b3da23e4a2cc22b230ca34a62b792ad09cea027 for the actually interesting change here.

PeterJCLaw commented 5 years ago

Thanks for the review. I'm actually tempted to hold off merging this for the moment as it's semi-breaking -- if I were to start using this and add a README, then users of older versions would get broken.

RealOrangeOne commented 5 years ago

(Reviving due to discussion in https://github.com/srobo/inventory/pull/10)

This is definitely a breaking change, but it's quite an important one. I'm not sure how we go about rolling this out in a clean way, so it might be best to just ship it and ask people to update. What do the failures look like for older clients, outside of inv-validate erroring?

PeterJCLaw commented 5 years ago

I know that Rich has expressed in the past a desire to extract the inventory tools from the sr.tools, which I had ben pondering as a good time to introduce a breaking change.

I'm not actually sure if that's a good idea and I had been considering ways to do it which made one or other (or both) parts library-like such that they could still interoperate.

I can't recall what the failure modes are for older cilents when we add a README file, however I doubt that they're helpful. I did consider adding something which would improve the error message, however that becomes a bit chicken & egg (other, perhaps, than in the breakout scenario described above).

trickeydan commented 5 years ago

I know that Rich has expressed in the past a desire to extract the inventory tools from the sr.tools, which I had ben pondering as a good time to introduce a breaking change.

I think that extracting the inventory tools from sr.tools is a good idea. The vast majority of volunteers do not use any other functionality.

I'm not actually sure if that's a good idea and I had been considering ways to do it which made one or other (or both) parts library-like such that they could still interoperate.

Sure.

I can't recall what the failure modes are for older cilents when we add a README file, however I doubt that they're helpful. I did consider adding something which would improve the error message, however that becomes a bit chicken & egg (other, perhaps, than in the breakout scenario described above).

I'd encourage increasing the major version number for this change.

trickeydan commented 2 years ago

Hi @PeterJCLaw, what are the next steps to get this moved forward?

Perhaps we could update the tool, wait a period for volunteers to update (say 6 months, with comms?) and then add a README to the inventory repo?

PeterJCLaw commented 2 years ago

Hi @PeterJCLaw, what are the next steps to get this moved forward?

Perhaps we could update the tool, wait a period for volunteers to update (say 6 months, with comms?) and then add a README to the inventory repo?

Yeah, something like that. Or we could revisit the idea of extracting the inventory tooling.