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 new cs:condition element to choose #260

Merged
merged 1 commit into from Jun 22, 2020
Merged

Add new cs:condition element to choose #260

merged 1 commit into from Jun 22, 2020

Conversation

bdarcus
Copy link
Member

@bdarcus bdarcus commented Jun 18, 2020

Description

This adds a modified version of the CSL-M condition element as an option
to configure more complex conditional rules.

Also splits off the cs:choose section into a separate file.

Closes #255

RFC

Could people please test this, both the actual code, and also the docstrings, in a validating edtior (like Atom), and post feedback?

Will it be totally clear for style authors how this would work if we merge this?

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@bdarcus bdarcus changed the base branch from master to v1.1 June 18, 2020 12:34
@bdarcus bdarcus changed the base branch from v1.1 to master June 18, 2020 12:35
@bdarcus
Copy link
Member Author

bdarcus commented Jun 18, 2020

PS - I'm getting confused by working with the different base branches.

@bwiernik

This comment has been minimized.

@bdarcus

This comment has been minimized.

schemas/styles/csl-choose.rnc Outdated Show resolved Hide resolved
schemas/styles/csl-choose.rnc Show resolved Hide resolved
schemas/styles/csl-choose.rnc Outdated Show resolved Hide resolved
@bdarcus
Copy link
Member Author

bdarcus commented Jun 18, 2020

I resolved your requested changes @bwiernik, but I'm not sure how to formally do that in the review system ATM.

Also rebased.

In any case, should be ready to test.

@bdarcus
Copy link
Member Author

bdarcus commented Jun 18, 2020

@bdarcus This would go into v1.1, no?

Yep; this is what I mean above by getting confused; when I switched the base branch.

But we can figure that out later.

@bwiernik - if you have time in the next few days, can you please figure out how to properly get this setup for merging in v1.1?

I'm hoping to we can close 1.0.2 work ASAP to avoid some of this confusion as we wrap up 1.1.

@bwiernik bwiernik changed the base branch from master to v1.1 June 18, 2020 19:19
@bwiernik
Copy link
Member

Just set the base branch to v1.1 (I just did that).

@bdarcus
Copy link
Member Author

bdarcus commented Jun 18, 2020

How weird; when I did that earlier, it resulted in some funky results. Maybe subsequent changes resolved that.

@bdarcus
Copy link
Member Author

bdarcus commented Jun 18, 2020

How weird; when I did that earlier, it resulted in some funky results. Maybe subsequent changes resolved that.

Actually, now i see what I earlier saw, which confuses me.

See all those commits on this PR?

It looks to me like this has become the new 1.1 branch!

Also, I had to commit a white space fix, thinking I would just squash commit this PR, but that doesn't appear possible now, without screwing things up further, or losing history?

@bdarcus bdarcus modified the milestones: CSL 1.0.2, CSL 1.1 Jun 18, 2020
@bwiernik
Copy link
Member

How weird; when I did that earlier, it resulted in some funky results. Maybe subsequent changes resolved that.

Actually, now i see what I earlier saw, which confuses me.

See all those commits on this PR?

It looks to me like this has become the new 1.1 branch!

No, I don't see the commits in this PR on the v1.1 branch: https://github.com/citation-style-language/schema/commits/v1.1

@bdarcus
Copy link
Member Author

bdarcus commented Jun 19, 2020

To fix (this might be helpful for someone in the future), I:

  1. checked out the v1.1 branch
  2. checked out a new local branch from there
  3. cherry-picked the commit specific to this PR
  4. overwrote my new local branch against this remote one with git push origin +choose-conditions-new:condition-element
  5. deleted the local "new" branch

This adds a modified version of the CSL-M conditions element and
condition child as an option to configure more complex conditional
rules.

Also splits off the cs:choose section into a separate file.

Closes #255
@bdarcus
Copy link
Member Author

bdarcus commented Jun 20, 2020

Anyone have a chance to test this?

I did, but am not the best person to test it.

Hoping to merge later today or tomorrow.

@denismaier
Copy link
Member

denismaier commented Jun 20, 2020 via email

@bwiernik
Copy link
Member

bwiernik commented Jun 20, 2020

Bruce, when you find that your fork or local copy of a repo is screwed up, it’s often best to reset from the upstream master:

git reset --hard upstream/master

@bdarcus bdarcus merged commit 6b39bcb into v1.1 Jun 22, 2020
@bdarcus
Copy link
Member Author

bdarcus commented Jun 22, 2020

I decided to merge it. We'll have to test 1.1 anyway.

@bdarcus bdarcus deleted the condition-element branch June 22, 2020 10:47
@denismaier
Copy link
Member

I tried to test v1.1, but I have errors here. Does that branch work for you?

@bdarcus
Copy link
Member Author

bdarcus commented Jun 22, 2020 via email

@bdarcus
Copy link
Member Author

bdarcus commented Jun 22, 2020 via email

@bdarcus
Copy link
Member Author

bdarcus commented Jun 22, 2020

I think there's another bug.

@bdarcus
Copy link
Member Author

bdarcus commented Jun 22, 2020

Now fixed.

@denismaier
Copy link
Member

Ok, it seems to work now.

@denismaier
Copy link
Member

But: we still need to wrap the the conditions under cs:conditions, right? Haven't we agreed that this is not strictly necessary?

@bdarcus
Copy link
Member Author

bdarcus commented Jun 23, 2020 via email

@denismaier
Copy link
Member

Auto-complete should require the cs:citations wrapper.

Currently, yes. But is that on purpose? I think we've said we can do without.

@bdarcus
Copy link
Member Author

bdarcus commented Jun 23, 2020 via email

@denismaier
Copy link
Member

His main point was that the conditions should appear an bloc at the beginning. That would also be required without the additional element. But I can live with that as well.

@bwiernik
Copy link
Member

Yeah, I don't care one way or the other so long as all condition elements appear together.

bwiernik pushed a commit to bwiernik/schema that referenced this pull request Jul 8, 2020
This adds a modified version of the CSL-M conditions element and
condition child as an option to configure more complex conditional
rules.

Also splits off the cs:choose section into a separate file.

Closes citation-style-language#255
bdarcus added a commit that referenced this pull request Jul 26, 2020
This adds a modified version of the CSL-M conditions element and
condition child as an option to configure more complex conditional
rules.

Also splits off the cs:choose section into a separate file.

Closes #255
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

extend condition syntax
3 participants