Skip to content

Fix build warnings & add CI#628

Merged
sophiajt merged 3 commits intonushell:masterfrom
max-sixty:build-warnings
Sep 11, 2019
Merged

Fix build warnings & add CI#628
sophiajt merged 3 commits intonushell:masterfrom
max-sixty:build-warnings

Conversation

@max-sixty
Copy link
Copy Markdown
Contributor

I don't know the practices in this project so please lmk if this is reasonable

@sophiajt
Copy link
Copy Markdown
Contributor

sophiajt commented Sep 9, 2019

@max-sixty - thanks for the PR! It will be good to fix these warnings.

For now, on Azure Pipelines we're experimenting with only building the default build to help with build times. We have another build which is run nightly (on Circle CI I believe?) that just got turned on that will test all the features.

A couple options:

We could turn the error-on-warning flag for one of the platforms (maybe the fastest one, which I think is macOS). Or, we could see if we could add it to the nightly build.

@max-sixty
Copy link
Copy Markdown
Contributor Author

Great—as you wish re the options.

Out of interest, do you have tests that only run on --all-features? Do you ever merge code that fails those? (possible I'm misunderstanding something basic about how that works...)

@sophiajt
Copy link
Copy Markdown
Contributor

@max-sixty we currently don't have tests for the optional features but it's definitely something we should add

# Conflicts:
#	src/commands/config.rs
@max-sixty
Copy link
Copy Markdown
Contributor Author

@max-sixty we currently don't have tests for the optional features but it's definitely something we should add

Once they're added, will you need --all-features anyway?

@wycats
Copy link
Copy Markdown
Contributor

wycats commented Sep 10, 2019

@jonathandturner Could we parallelize the tests by feature? I don't like the idea of merging code to master that doesn't work, only to find it out the next day and having to do a revert or hotfix.

@sophiajt
Copy link
Copy Markdown
Contributor

@wycats could you say more? Tests are run in parallel by default in Rust, though I'm not sure what you mean by "by feature".

@sophiajt
Copy link
Copy Markdown
Contributor

@wycats one idea would be to do an all-features test run on one of the platforms (like macOS, since it's fastest) rather than doing it on all 3. The build will take the same amount of time, but we'll have some coverage for the optional features.

@sophiajt
Copy link
Copy Markdown
Contributor

Since we're seeing breakage from not testing all features, the approach in this PR is seeming like the important part.

We can work on build times, but correctness is the more important foundation to start from.

@sophiajt sophiajt merged commit f05c7d6 into nushell:master Sep 11, 2019
@max-sixty max-sixty deleted the build-warnings branch September 11, 2019 12:37
@max-sixty max-sixty mentioned this pull request Sep 11, 2019
elferherrera pushed a commit to elferherrera/nushell that referenced this pull request Feb 7, 2022
…#628)

* get_columns is working in the columns command

* the new location of the get_columns method is nu-protocol/src/column.rs

* reference the new location of the get_columns method

* move get_columns to nu-engine
kubouch pushed a commit that referenced this pull request Feb 7, 2022
* get_columns is working in the columns command

* the new location of the get_columns method is nu-protocol/src/column.rs

* reference the new location of the get_columns method

* move get_columns to nu-engine
Hofer-Julian pushed a commit to Hofer-Julian/nushell that referenced this pull request Jan 27, 2023
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.

3 participants