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

Make SHORTNAME detection on GitHub Actions much simpler #326

Closed
wants to merge 1 commit into from

Conversation

foolip
Copy link
Member

@foolip foolip commented Jun 12, 2020

The complicated thing is still used locally and on Travis.

The complicated thing is still used locally and on Travis.
@foolip
Copy link
Member Author

foolip commented Jun 12, 2020

Here I also organized things a bit into constants, environment variables and local variables.

@annevk
Copy link
Member

annevk commented Jun 12, 2020

So we basically have this because of compat/console/streams not using their shortname as their .bs name, right? @domenic any thoughts on renaming them instead?

@domenic
Copy link
Member

domenic commented Jun 12, 2020

Renaming is quite disruptive and loses the entire file history in the GitHub UI. I think adding a single line in the build script is a very small price to pay.

(I'm not really sure why we'd expand that single line into 5 lines as this PR does, though...)

@annevk
Copy link
Member

annevk commented Jun 12, 2020

Well, you wouldn't lose history for the links we advocate, e.g., https://github.com/whatwg/compat/commits, and I think blame isn't impacted either, but that's fair.

@foolip
Copy link
Member Author

foolip commented Jun 12, 2020

I'm not really sure why we'd expand that single line into 5 lines as this PR does, though...

Things work without this change, but interpreting that sed command is very hard. This is a follow-up to #325 to make it more obvious why it's like that (in the future: for local builds only). But if this just looks more convoluted, I could either:

  • abandon this
  • just use the directory name in the else branch

@annevk
Copy link
Member

annevk commented Jun 12, 2020

I would abandon, but explain the complexity in a comment.

@domenic
Copy link
Member

domenic commented Jun 12, 2020

Given that the sed command still exists, it doesn't feel like a win to just add a second path where the sed command is bypassed. Now one has to understand complicated thing + simple thing, which seems slightly worse than having to understand complicated thing by itself.

@foolip
Copy link
Member Author

foolip commented Jun 12, 2020

I've sent #327 instead.

@foolip foolip closed this Jun 12, 2020
@foolip foolip deleted the simpler-shortname branch June 12, 2020 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants