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

Add build provider #277

Merged
merged 4 commits into from
Dec 30, 2021
Merged

Add build provider #277

merged 4 commits into from
Dec 30, 2021

Conversation

starbelly
Copy link
Member

This is ready but depends on #272 being merged first.

Closes #233

@starbelly starbelly force-pushed the add-build-provider branch 7 times, most recently from b395dfe to 959e460 Compare December 28, 2021 19:36
@starbelly starbelly marked this pull request as ready for review December 28, 2021 19:37
@starbelly starbelly requested a review from ferd December 28, 2021 19:37
%% <li>`links' - A map where the key is a link name and the value is the link URL. Optional but highly recommended.
%% <li> `files' - A list of files and directories to include in the package. Defaults to standard project directories,
%% so you usually don't need to set this property.</li>
%% <li> `include_files' - A list of paths containing files you wish to include in a release. </li>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like these should be called include_paths and exclude_paths and we could also perhaps support exclude_files. They both work on paths right now. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the _paths variants are clearer, but it's not a super significant difference either way, especially if this ends up using glob or wildcard patterns (though that's exclude_patterns, which has also been renamed). The "confusion" is already common (e.g. filelib supports is_file|dir|regular already).

I'd tend to still lower the updating friction for the plugin by keeping this compatible with the stuff that was already in the .app.src file though, to make upgrading the plugin as easy as possible. The 7.0 update will likely have people update their licenses, so there should at least be an obvious way to know that you're done fixing what broke rather than suddenly having older options not work (if any, I mostly recall just using files).

Either way I'm fine with the direction we're going.

%% <h2> Command line options </h2>
%%
%% <ul>
%% <li>`--repo' - Specify the repository to use in the task. This option is required when
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that we support this on this provider, but we currently support being able to configure a doc provider on a per repo basis.

%% <li>`links' - A map where the key is a link name and the value is the link URL. Optional but highly recommended.
%% <li> `files' - A list of files and directories to include in the package. Defaults to standard project directories,
%% so you usually don't need to set this property.</li>
%% <li> `include_files' - A list of paths containing files you wish to include in a release. </li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the _paths variants are clearer, but it's not a super significant difference either way, especially if this ends up using glob or wildcard patterns (though that's exclude_patterns, which has also been renamed). The "confusion" is already common (e.g. filelib supports is_file|dir|regular already).

I'd tend to still lower the updating friction for the plugin by keeping this compatible with the stuff that was already in the .app.src file though, to make upgrading the plugin as easy as possible. The 7.0 update will likely have people update their licenses, so there should at least be an obvious way to know that you're done fixing what broke rather than suddenly having older options not work (if any, I mostly recall just using files).

Either way I'm fine with the direction we're going.

%% <ul>
%% <li>`--repo' - Specify the repository to use in the task. This option is required when
%% you have multiple repositories configured, including organizations. The argument must
%% be a fully qualified repository name (e.g, `hexpm', `hexpm:my_org', `my_own_hexpm').
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PRobably worth adding defaults to expm

%% contains all needed files before publishing. See --output below for setting the output path.
%% </li>
%% <li> `-o', `--output' - Sets output path. When used with --unpack it means the directory
%% (Default: <app>-<version>). Otherwise, it specifies tarball path (Default: <app>-<version>.tar)</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would usually tend to default the output to somewhere in _build/<profile>/ so these don't get to bypass .gitignore on build artifacts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Will change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@starbelly
Copy link
Member Author

the _paths variants are clearer, but it's not a super significant difference either way, especially if this ends up using glob > or wildcard patterns (though that's exclude_patterns, which has also been renamed). The "confusion" is already common (e.g. filelib supports is_file|dir|regular already).

I'll add support for _paths but keep in support for the old name.

- add support for building packages and docs tarballs
- align the keys for packages and docs in regards to `version` vs `vsn`
- rename attribute for exclude regular expressions from `exclude_regexps` to `exclude_patterns`
@ferd
Copy link
Member

ferd commented Dec 29, 2021

yeah just supporting newer options without breaking the old stuff is fine, if it's not too much extra complexity. Keep a comment about the reason for it so whoever is still maintaining this in the future has a good contextual clue of the intent behind it.

@starbelly
Copy link
Member Author

Note : I made one more change, in line with the others around name changes (i.e., continue to support exclude_regexps).

@starbelly starbelly merged commit acf0d31 into erlef:master Dec 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for exclude_patterns?
2 participants