Use RELATIVE_PROJECT_PATH in export #28

Bluenix2 · 2020-07-23T14:07:16Z

Here we assume preset.export_path is relative to the project path, which should be the case since this is set up in the editor were the project is the root.
Fixes #25, no longer needing ../../../.local/share/godot/builds/ to reach it during deployment.

This assumes presets.export_path is relative to RELATIVE_PROJECT_PATH.

This means we use the whole path relative to RELATIVE_PROJECT_PATH rather than just filename.

firebelley

Thanks for the contribution! I have some thoughts about the change that I'd like your response to. Also, you'll need to run a yarn build so that the js file is generated with the changes you make.

src/godot.ts

As preparation for a new approach

Update moveBuildsToExportDirectory to use new USE_PRESET_EXPORT_PATH constant

Update main file to call moveBuildsToExportDirectory if USE_PRESET_EXPORT_PATH is true

Bluenix2 · 2020-07-23T21:59:21Z

Okay, I think I found the best way to implement this. I am also happy with the documentation.
But you needed me to build with yarn? Let me clone this and do that.

firebelley · 2020-07-23T22:15:09Z

Awesome! Yeah if you aren't familiar with Yarn you can check it out here https://yarnpkg.com/

Once you have it installed do a yarn install then yarn build and commit the output file. Then once that's good to go, I can do some testing and review, but so far the draft looks great!

Bluenix2 · 2020-07-23T22:46:48Z

That should be all!
I had to fix two linting problems ESLint didn't like (mostly regarding line length).

Should be all done now. But there is one thing I would like to discuss. And that is changing this new constant to default to true. I set it to false now while working but I think it should actually be changed.
Pros: Expected behaviour since you define it in export_presets.cfg. You don't repeat yourself because of this, good design.
Cons: Breaking change because this new constant will overwrite and be prioritized over relative_export_path. This could be changed thus making relative_export_path higher in priority.

Bluenix2 · 2020-07-23T23:00:16Z

Thinking through the pros and cons I realized that it could be good to document the current bias.

firebelley · 2020-07-24T00:24:04Z

So I have a few thoughts, totally open to doing what you want to do but here are some things for consideration:

Defaulting the new property to true is a breaking change, which would require a major version upgrade (not a big deal but something to consider)
- Maybe we should consider getting rid of the relative_export_path altogether if we want to make this a major version upgrade, and just inform users that they'll have to use the path in export presets
- This could be an issue for users with lots of presets (I have 8 configured presets on one of my projects!)
- Perhaps the relative_export_path could be repurposed as yet another location where all builds get moved? So users could have the option of dumping all builds in a single location regardless of what's configured in their export presets?
If we want to keep this to a minor version upgrade, I think it pretty much has to be the case that the default value is of the new property is false so that existing users can upgrade without "breaking" changes
- In this case I think it's definitely smart to document that the new property takes precedence over relative_export_path
- Keeping this to a minor version upgrade doesn't preclude us from making this default behavior in the future with a major version upgrade. This might be preferable because at the moment I am kind of in the dark about how everyone is using this action and so can't make informed decisions about the best default user experience.

Basically, if we want to get this change in without too many changes we'd have to keep the value defaulted to false and do a minor version upgrade. However, if you'd be OK with me (or you!) taking the opportunity to rework some things as part of a major version upgrade I would be totally open to that, with the caveat that this feature would then go out at some point in the future. And then there's a middle road where this goes in now, and becomes default behavior in a future version.

I'm totally fine with whatever decision you choose to make, so just let me know! I'll be leaving a comment or two on the code and I'll have time to test the change tomorrow.

Nice work, and thanks again for the contribution!

firebelley

Looks great overall! The only blocking thing is that the new input needs to be defined in action.yml. See my comments for more info

src/constants.ts

src/file.ts

Bluenix2 · 2020-07-24T13:54:37Z

Regarding breaking change, I say we merge it like this and I'll open another pull request that changes the default and removes relative_export_path that we can merge when a complete new version is about to be published (3.x)

firebelley · 2020-07-24T14:00:13Z

Ok, sounds good! Please make sure you've built your latest changes with yarn build and I'll do some testing tonight. If all goes well I'll go ahead and merge the change and create a new version 👍

Bluenix2 · 2020-07-24T19:56:46Z

Alright, I built again!

firebelley

I just completed my testing and it works really well 👍 I just have one thing that I missed yesterday, as soon as you address that I'll merge this change in and create a new release!

src/file.ts

Includes build

Bluenix2 · 2020-07-24T21:08:45Z

Fixed that spammy message now, cd9b652 includes build.

firebelley

Perfect, thanks again for your contribution!

Bluenix2 added 3 commits July 23, 2020 15:53

Change executablePath from buildDir

0d4172d

This assumes presets.export_path is relative to RELATIVE_PROJECT_PATH.

Remove path.basename call

f6f32a2

This means we use the whole path relative to RELATIVE_PROJECT_PATH rather than just filename.

Merge branch 'patch-1' into patch-2

b109b1e

firebelley reviewed Jul 23, 2020

View reviewed changes

src/godot.ts Outdated Show resolved Hide resolved

Bluenix2 marked this pull request as draft July 23, 2020 17:12

Bluenix2 added 6 commits July 23, 2020 19:50

Revert my previous changes

b67e7ff

As preparation for a new approach

Add USE_PRESET_EXPORT_PATH input and constant

3b2e70f

Update moveBuilds.. to use USE_PRESET_EXPORT_PATH

be6bfdd

Update moveBuildsToExportDirectory to use new USE_PRESET_EXPORT_PATH constant

Update main to move builds if USE_PRESET... is true

2bd2ffe

Update main file to call moveBuildsToExportDirectory if USE_PRESET_EXPORT_PATH is true

Add use_preset_export_path

c151547

Remove trailing filename from path using dirname()

b1a8faf

Bluenix2 added 3 commits July 24, 2020 00:28

Export USE_PRESET_EXPORT_PATH

d9b5561

Fix linting problems

a039e7b

Add index.js generated by ncc

c407eab

Bluenix2 marked this pull request as ready for review July 23, 2020 22:41

Document bias towards use_preset_export_path

3f0aed2

firebelley requested changes Jul 24, 2020

View reviewed changes

src/constants.ts Outdated Show resolved Hide resolved

src/file.ts Show resolved Hide resolved

Bluenix2 commented Jul 24, 2020

View reviewed changes

src/file.ts Show resolved Hide resolved

Bluenix2 added 2 commits July 24, 2020 14:03

Add new field to action.yml

4f860d1

Correctly type use_preset_export_path

d51b273

New build

eb63ace

firebelley requested changes Jul 24, 2020

View reviewed changes

src/file.ts Outdated Show resolved Hide resolved

Fix spammy message

cd9b652

Includes build

firebelley approved these changes Jul 24, 2020

View reviewed changes

firebelley merged commit 22b5f8b into firebelley:master Jul 24, 2020

Bluenix2 deleted the patch-2 branch July 24, 2020 22:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use RELATIVE_PROJECT_PATH in export #28

Use RELATIVE_PROJECT_PATH in export #28

Bluenix2 commented Jul 23, 2020

firebelley left a comment

Bluenix2 commented Jul 23, 2020

firebelley commented Jul 23, 2020 •

edited

Loading

Bluenix2 commented Jul 23, 2020

Bluenix2 commented Jul 23, 2020

firebelley commented Jul 24, 2020

firebelley left a comment

Bluenix2 commented Jul 24, 2020

firebelley commented Jul 24, 2020

Bluenix2 commented Jul 24, 2020

firebelley left a comment

Bluenix2 commented Jul 24, 2020

firebelley left a comment

Use RELATIVE_PROJECT_PATH in export #28

Use RELATIVE_PROJECT_PATH in export #28

Conversation

Bluenix2 commented Jul 23, 2020

firebelley left a comment

Choose a reason for hiding this comment

Bluenix2 commented Jul 23, 2020

firebelley commented Jul 23, 2020 • edited Loading

Bluenix2 commented Jul 23, 2020

Bluenix2 commented Jul 23, 2020

firebelley commented Jul 24, 2020

firebelley left a comment

Choose a reason for hiding this comment

Bluenix2 commented Jul 24, 2020

firebelley commented Jul 24, 2020

Bluenix2 commented Jul 24, 2020

firebelley left a comment

Choose a reason for hiding this comment

Bluenix2 commented Jul 24, 2020

firebelley left a comment

Choose a reason for hiding this comment

firebelley commented Jul 23, 2020 •

edited

Loading