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

proposal/SetColumnConfigNumberless #302

Closed

Conversation

Skeeve
Copy link
Contributor

@Skeeve Skeeve commented Mar 11, 2024

Proposed Changes

SetColumnConfig could be position-based.

So the n-th (Zero-based) ColumnConfig would set the (n+1)-th Column's config.

That way it's not required to pass a "Number" or "Name".

Copy link

sonarcloud bot commented Mar 11, 2024

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@coveralls
Copy link

Pull Request Test Coverage Report for Build 8229785862

Details

  • 7 of 8 (87.5%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.03%) to 99.971%

Changes Missing Coverage Covered Lines Changed/Added Lines %
table/render_init.go 7 8 87.5%
Totals Coverage Status
Change from base Build 7701492032: -0.03%
Covered Lines: 3433
Relevant Lines: 3434

💛 - Coveralls

@jedib0t
Copy link
Owner

jedib0t commented Mar 11, 2024

Hey @Skeeve. I appreciate the contribution. However this change will make ColumnConfigs very ambiguous and will possibly introduce unintentional bugs. Basically this change makes it so a ColumnConfig struct with Number: 0 behaves the same as Number: 1.

Consider what happens when there are two column configs like the following:

    tw.SetColumnConfigs([]table.ColumnConfig{
        {Number: 1, Align: text.AlignLeft},  // Number==1
        {Align: text.AlignRight},            // Number==0
    })

In the above, the not-well defined ColumnConfig with Number==0 will take effect, and the well-defined ColumnConfig with Number==1 will be ignored.

The ideal change would be to change the interface for Render() return an error in two cases:

  • When there are multiple ColumnConfigs defined for the same column
  • [and/or] When there are ColumnConfigs with no Number and no Name

The reason for the error being in Render() is because SetColumnConfigs could be called before AppendHeader, and the Name values may not mean anything until Render() is called. Regardless, this would need a version bump since it will be a backward incompatible change.

I'll consider that approach the next time I bump the major version, but for now I'll have to decline this change.

@jedib0t jedib0t closed this Mar 11, 2024
@Skeeve
Copy link
Contributor Author

Skeeve commented Mar 13, 2024

Consider what happens when there are two column configs like the following:

    tw.SetColumnConfigs([]table.ColumnConfig{
        {Number: 1, Align: text.AlignLeft},  // Number==1
        {Align: text.AlignRight},            // Number==0
    })

In the above, the not-well defined ColumnConfig with Number==0 will take effect, and the well-defined ColumnConfig with Number==1 will be ignored.

That's correct and expected. Same would happen if you set Number: 1 instead of leaving the number out.

It's simply a convenience for not having to write the numbers. One shouldn't mix.

@Skeeve Skeeve deleted the proposal/SetColumnConfigsNumberless branch March 15, 2024 06:11
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.

None yet

3 participants